Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEV: Making _RBV PVs read-only #328

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

KaushikMalapati
Copy link
Contributor

https://jira.slac.stanford.edu/browse/ECS-5005

This adds an ASG field with the value NO_WRITE to any EPICSRecord with direction input in the jinja database template. This access security group will be added to a new version of ads-ioc and will only allow reading, not writing to pvs.

pcdshub/ioc-common-ads-ioc#103

@ZLLentz
Copy link
Member

ZLLentz commented Jul 20, 2024

My tiny thoughts on this side are:

  • Is it possible to write a useful test for this in test_record.py? Well, I can think of some tests to run, but are any of them useful?
  • Does this create some backcompat issues while using a newer pytmc to build an old ads-ioc? Is there anywhere here we can or should be checking for version compatibilities?

@KaushikMalapati
Copy link
Contributor Author

My tiny thoughts on this side are:

  • Is it possible to write a useful test for this in test_record.py? Well, I can think of some tests to run, but are any of them useful?
  • Does this create some backcompat issues while using a newer pytmc to build an old ads-ioc? Is there anywhere here we can or should be checking for version compatibilities?

I added some simple test cases. Also, when I edited the jinja template I assumed that direction would always be "input" or "output." Is that true, or can it have any other values like "i," "o," or "io?"

As far as backward compatibility, I tested that having an ASG field whose value is not specified does not break anything, so there shouldn't be any problems using a newer version of pytmc with an older version of ads-ioc.

@ZLLentz
Copy link
Member

ZLLentz commented Jul 23, 2024

Also, when I edited the jinja template I assumed that direction would always be "input" or "output." Is that true, or can it have any other values like "i," "o," or "io?"

Skimming the code, I agree that this should always be "input" or "output" if you go through the full chain but this doesn't seem to be explicitly enforced everywhere... it's worth testing briefly with a real IOC where the user may have used any combinations of these specifiers.

@KaushikMalapati
Copy link
Contributor Author

KaushikMalapati commented Sep 14, 2024

Also, when I edited the jinja template I assumed that direction would always be "input" or "output." Is that true, or can it have any other values like "i," "o," or "io?"

Skimming the code, I agree that this should always be "input" or "output" if you go through the full chain but this doesn't seem to be explicitly enforced everywhere... it's worth testing briefly with a real IOC where the user may have used any combinations of these specifiers.

record = EPICSRecord(

record = EPICSRecord(
pvname,
self.input_rtyp,
"input",
fields=self.field_defaults,
autosave=self.autosave_defaults.get("input"),
aliases=aliases,
archive_settings=self.archive_settings,
package=self,
)
...
record = EPICSRecord(
self.pvname,
self.output_rtyp,
"output",
fields=self.field_defaults,
autosave=self.autosave_defaults.get("output"),
aliases=self.aliases,
archive_settings=self.archive_settings,
package=self,
)
Looks like it will always be "input" or "output"

Do you know why I'm failing the tests on 3.12?

@KaushikMalapati KaushikMalapati marked this pull request as ready for review September 14, 2024 07:28
@ZLLentz
Copy link
Member

ZLLentz commented Sep 16, 2024

I do not know why you're failing the test on python 3.12 but I do know that it probably has nothing to do with this PR

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pytmc diff here is simple and effective when paired with the ads-ioc PR

@ZLLentz
Copy link
Member

ZLLentz commented Sep 16, 2024

I'm happy to merge this and get a new pytmc tag out if we're satisfied here

@KaushikMalapati KaushikMalapati merged commit c30a7c9 into pcdshub:master Sep 16, 2024
9 of 11 checks passed
@KaushikMalapati KaushikMalapati deleted the readonly branch December 1, 2024 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants