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

merge secretrefs and configrefs into single list #81

Merged
merged 33 commits into from
Jan 15, 2025
Merged

merge secretrefs and configrefs into single list #81

merged 33 commits into from
Jan 15, 2025

Conversation

fabianburth
Copy link
Contributor

What this PR does / why we need it

This PR merges config ref and secret ref into a single list and exchanges the implicit defaulting mechanism (implicitly inheriting the configuration of the previous object in the pipeline - e.g. component inherits from repository) with an explicit mechanism.

Merging into single list
The order in which the configuration is applied is relevant. This becomes confusing and cumbersome with two separate lists and additional mechanisms would be needed.

Changing the defaulting mechanism

  1. The previous defaulting mechanism would automatically inherit the referenced config maps if the config maps were empty and the secrets if the secrets are empty. This does not make too much sense (but was overseen, since originally, this was already intended as a single list). If a user would provide all his configuration data in secrets and therefore, doesn't specify any config maps, the mechanism would still have inherited the config maps and potentially overwritten some of the configs provided in the secret.
  2. The new mechanism allows to only inherit parts of the configuration. Thus, inherit the reference to config map a BUT NOT the reference to config map b. This is useful, because the e.g. the Replication needs the same credentials as the Component (since it needs to have access to the component), but it does not need other parts of the configuration such as the signing keys.

Which issue(s) this PR fixes

#80

@fabianburth fabianburth requested a review from a team as a code owner December 9, 2024 16:06
jakobmoellerdev
jakobmoellerdev reviewed Dec 17, 2024
View reviewed changes
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

Mostly looking good, just nits. generally looking good

@fabianburth fabianburth force-pushed the ocm-config branch 2 times, most recently from 0878b2d to e6d4fd4 Compare January 13, 2025 14:17
jakobmoellerdev
jakobmoellerdev reviewed Jan 13, 2025
View reviewed changes
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

Just one question on the Spec for the API, apart from that LGTM

@fabianburth
merge secretrefs and configrefs into configuration
0b23df3 
# Conflicts:
#	api/v1alpha1/component_types.go

# Conflicts:
#	api/v1alpha1/common_types.go

# Conflicts:
#	api/v1alpha1/ocmrepository_types.go
#	config/crd/bases/delivery.ocm.software_ocmrepositories.yaml
@fabianburth
feat: add cel validation rules to OCMConfiguration
98fcd49
@fabianburth
feat: add default policy
d27197f
@fabianburth
chore: remove obsolete interfaces
e20a9d6
@fabianburth
chore: remove duplicate comments
e914dee
@fabianburth
chore: remove obsolete slice clones
025705c
@fabianburth
fix: add missing error return
f820e99
@fabianburth
chore: refactor verifis
7ce8ca2
@fabianburth
docs: adjust examples
2e35613
@fabianburth
docs: remove configuration policy defaulting from code
d97a9b8 
The propagation policy now is defaulted through a kubebuilder directive which makes this logic redundant.
@fabianburth
fix: effective ocm config in the ocm resource status
b9c01a3
@fabianburth
feat: adjust replication controller to reworked config handling
736f392
@fabianburth
feat: add replication to config providers
9cfa3b5
@fabianburth
feat: inline ref in ocm configuration
231b7e6
@fabianburth
chore: refactor configure context
2909333
@fabianburth
chore: refactor get propagated ocm config to get effective ocm config
9924b60 
The GetEffectiveOCMConfig() is merely a getter, which is much more easy and stable to maintain in our public API.
@fabianburth
chore: add line length linter (lll) exception for kubebuilder statements
cba2e40
@fabianburth
chore: fix linter issues
2307bf4
@fabianburth
feat: adjust e2e tests to reworked config handling
14e8a57
@fabianburth
feat: adjust e2e tests to reworked config handling
4bba6ee
@fabianburth
feat: refine kubebuilder validations
c532d66
@fabianburth
fix: correct examples
ba00d3d
@fabianburth
feat: adjust e2e tests to new config handling
89f8f66
jakobmoellerdev
jakobmoellerdev reviewed Jan 14, 2025
View reviewed changes
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

LGTM,
@frewilhelm PTAL

frewilhelm
frewilhelm reviewed Jan 14, 2025
View reviewed changes
Copy link
Contributor

@frewilhelm frewilhelm left a comment

Choose a reason for hiding this comment

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

only a preliminary review..

@fabianburth fabianburth merged commit 1b31e29 into main Jan 15, 2025
4 checks passed
@fabianburth fabianburth deleted the ocm-config branch January 15, 2025 09:04
@frewilhelm frewilhelm mentioned this pull request Jan 28, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakobmoellerdev jakobmoellerdev jakobmoellerdev left review comments

@frewilhelm frewilhelm frewilhelm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fabianburth @frewilhelm @jakobmoellerdev