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

Add site_defaults.json in site model directory, which can supply default values to all Metadata. #288

Merged
merged 24 commits into from
Apr 14, 2022

Conversation

johnrandolph
Copy link
Collaborator

No description provided.

@johnrandolph johnrandolph requested a review from grafnu April 6, 2022 17:42
Copy link
Collaborator Author

@johnrandolph johnrandolph left a comment

Choose a reason for hiding this comment

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

Ok ptal. Note that calling it "site_metadata" also looks good in code w/r/t variable names.

@@ -180,19 +196,25 @@
private final ExceptionMap exceptionMap;
private final String generation;
private final List<DeviceCredential> deviceCredentials = new ArrayList<>();
private final TreeMap<String, Object> siteMetadata;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like it would be cleaner overall to keep the class variable as Metadata (rather than TreeMap), and then convert to TreeMap only at the point of use?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then we convert it once per metadata loaded?

Copy link
Collaborator

Choose a reason for hiding this comment

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

True -- although I've never seen performance be an actual issue with any of this stuff :-). I also think it's only likely to be called relatively infrequently per device (once, maybe twice?) Your call, I'm good either way.

Copy link
Collaborator

@grafnu grafnu left a comment

Choose a reason for hiding this comment

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

Oh, "unapprove" -- was hoping to have the update for the udmi_site_model reference in the PR as well (for testing)...

@johnrandolph johnrandolph merged commit 5c7c061 into faucetsdn:master Apr 14, 2022
@johnrandolph johnrandolph deleted the site_wide_defaults branch April 14, 2022 19: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.

None yet

2 participants