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.

Sign up for GitHub

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

Jump to bottom

Add configuration object #379

Merged
merged 22 commits into from
Oct 1, 2024
Merged

Add configuration object #379

merged 22 commits into from
Oct 1, 2024

Conversation

christoph-hamm
Copy link
Contributor

@christoph-hamm christoph-hamm commented Sep 17, 2024 •
edited by inf17101
Loading

Issues: #267

Definition of Done

The PR shall be merged only if all items mentioned in CONTRIBUTING.md have been followed. In case an item is not applicable as described, please provide a short explanation in the description.

@christoph-hamm
Add config to state object
96a6508 
Issue-Id: #267
@christoph-hamm
Fix problem with tests not building for common
42e8679 
Before this fix it was not possible to run the tests from the common
folder. This also prohibited running individual test from Visual
Studio Code (e.g. for debugging).

Issue-Id: #267
@christoph-hamm
Improve YAML serialization of ConfigItem
da99bed 
Issue-Id: #267
@christoph-hamm
Add internal config object
e9b4217 
Issue-Id: #267
@christoph-hamm
Fix unit tests
43bf139 
Issue-Id: #267
@christoph-hamm
Fix clippy error about unequal enum sizes
848c0d6 
Issue-Id: #267
@christoph-hamm
Remove unused import
7c7ca19 
Issue-Id: #267
@christoph-hamm
Interpret None fields as default instead of failing
8309e0f 
Issue-Id: #267
@christoph-hamm
Add configs to filtered state used in the ank CLI
a954f8e
@christoph-hamm
Also update server state if non workload has been changed
09c3804
@christoph-hamm
Merge branch 'main' into 267_Add_configuration_object
5a41e83
@christoph-hamm
Add missing fields in tests
f599aec 
Issue-Id: #267
@christoph-hamm
Remove commented out code
7c391e1 
Issue-Id: #267
@christoph-hamm
Fix year in copyright header of new files
9610370 
Issue-Id: #267
@inf17101 inf17101 self-requested a review September 27, 2024 12:39
@inf17101 inf17101 mentioned this pull request Sep 27, 2024
Closed
10 tasks
inf17101
inf17101 reviewed Sep 27, 2024
View reviewed changes
Copy link
Contributor

@inf17101 inf17101 left a comment

Choose a reason for hiding this comment

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

First half of review is finished. Only the server part is left to review.

ank/src/filtered_complete_state.rs Outdated Show resolved Hide resolved
api/proto/ank_base.proto Outdated Show resolved Hide resolved
api/proto/ank_base.proto Outdated Show resolved Hide resolved
api/src/convert.rs Outdated Show resolved Hide resolved
api/src/convert.rs Outdated Show resolved Hide resolved
common/src/objects/config.rs Outdated Show resolved Hide resolved
common/src/objects/config.rs Outdated Show resolved Hide resolved
common/src/objects/config.rs Outdated Show resolved Hide resolved
common/src/objects/state.rs Show resolved Hide resolved
common/src/objects/stored_workload_spec.rs Outdated Show resolved Hide resolved
@inf17101
Copy link
Contributor

@christoph-hamm: With the current object construction the filtering of config items via filter masks is not possible.

inf17101
inf17101 reviewed Sep 30, 2024
View reviewed changes
common/src/test_utils.rs Outdated Show resolved Hide resolved
@inf17101
Copy link
Contributor

Review part II is done. Comments published.

christoph-hamm and others added 7 commits September 30, 2024 14:44
@christoph-hamm @inf17101
Apply suggestions from code review
29a45e1 
Co-authored-by: Oliver <42932060+inf17101@users.noreply.github.com>
@christoph-hamm @inf17101
Remove commented out code
989d3f4 
Issue-Id: #267
Co-authored-by: Oliver <42932060+inf17101@users.noreply.github.com>
@christoph-hamm
Add missing use statement
73a9ca2 
Issue-Id: #267
@christoph-hamm
Allow missing workloads in UpdateStateRequeset
a81ad0b 
Issue-Id: #267
@christoph-hamm
Fix configs in sample objects
8440e58 
Issue-Id: #267
@christoph-hamm
Merge branch 'main' into 267_Add_configuration_object
592efe2
@christoph-hamm
Add missing config
e2b632f 
Issue-Id: #267
@inf17101
Copy link
Contributor

inf17101 commented Oct 1, 2024

@christoph-hamm: With the current object construction the filtering of config items via filter masks is not possible.

The filtering still does not work for config items. I will have a look.

@inf17101
Copy link
Contributor

inf17101 commented Oct 1, 2024 •
edited
Loading

The filtering works but there is a serde flatten missing in the api/build.rs file. So the user has to enter the filter mask desiredState.configs.configs.config_item_key to filter for config values. I will fix that quickly.

@inf17101
Copy link
Contributor

inf17101 commented Oct 1, 2024

The filtering works but there is a serde flatten missing in the api/build.rs file. So the user has to enter the filter mask desiredState.configs.configs.config_item_key to filter for config values. I will fix that quickly.

Fixed by adding a flatten config into the build.rs file. Now the filtering works.

inf17101
inf17101 approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@inf17101 inf17101 left a comment

Choose a reason for hiding this comment

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

LGTM👍

@inf17101 inf17101 merged commit 009d568 into main Oct 1, 2024
10 checks passed
@inf17101 inf17101 deleted the 267_Add_configuration_object branch October 1, 2024 13:37
@inf17101 inf17101 added enhancement New feature or request. Issue will appear in the change log "Features" and removed ready for review labels Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inf17101 inf17101 inf17101 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request. Issue will appear in the change log "Features"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@christoph-hamm @inf17101