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 --local-config-method, merge local app components with remote ones by default #301

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

skarekrow
Copy link
Collaborator

Fixes #300

This should obviate the original behavior of merging the local apps in totality and instead patch the apps specifed with the component overrides.

Special thanks to @rsnyman and @bsquizza for their help with this.

bonfire/bonfire.py Outdated Show resolved Hide resolved
@bsquizz
Copy link
Collaborator

bsquizz commented Apr 5, 2023

While I was at it I also renamed the 'local' source mode to 'file' because 'local' is a misleading name.

The local config is always parsed, even in 'appsre' mode. Really, what 'local' mode meant was that bonfire would fetch its apps config using the file defined at the appsFile in config.yaml instead of using app-sre. That mode is not currently used in the platform tooling so I do not think renaming it will have large impact, and that mode may prove useful again in the future so I kept the ability to define an appsFile in the code.

Whether your source of truth is app-sre or the 'apps file', the --local-config-method will operate the same way when it comes to merging your locally defined apps with the remotely defined ones.

@bsquizz bsquizz requested a review from Victoremepunto April 5, 2023 21:38
@bsquizz
Copy link
Collaborator

bsquizz commented Apr 5, 2023

When we release this, we should tag this as v5 since this is technically making a "breaking change" -- although I suspect it will impact very few people... and for the people it does impact, they just need a heads up about the new CLI flag if they desire to return to the old config merging behavior.

@Victoremepunto
Copy link
Collaborator

@skarekrow @bsquizz Can we add some E2E tests to keep this behavior away of future possible regressions ? they'll also help understand the behavior, I believe looking at a simplified use case with expectations is clearer than anything.

Thanks!

@bsquizz
Copy link
Collaborator

bsquizz commented Apr 6, 2023

Latest commit keeps this all backward-compatible:

  1. default merge method remains 'override' as it was before
  2. the source named 'local' is kept around for now and there's a warning indicating that it is deprecated and has been renamed to 'file'

@bsquizz
Copy link
Collaborator

bsquizz commented Aug 2, 2023

We're going to submit an ADR to get buy-in from leads on changing the default merge method

@bsquizz
Copy link
Collaborator

bsquizz commented Aug 28, 2023

Latest commit keeps this all backward-compatible:

  1. default merge method remains 'override' as it was before
  2. the source named 'local' is kept around for now and there's a warning indicating that it is deprecated and has been renamed to 'file'

Commit 10b78b7 reverted since we'll get approval for the change in default behavior via ADR process.

@bsquizz bsquizz force-pushed the brandon/local_component_merging branch from ff0ffff to a2ae703 Compare August 28, 2023 18:24
@bsquizz bsquizz force-pushed the brandon/local_component_merging branch from fa42835 to 6e8c9c0 Compare September 19, 2023 23:26
@bsquizz bsquizz force-pushed the brandon/local_component_merging branch from 6e8c9c0 to 9b38868 Compare October 9, 2023 19:42
@bsquizz bsquizz changed the title Merge local app components with app-sre ones Add --merge-method, merge local app components with remote ones by default Oct 9, 2023
@bsquizz bsquizz changed the title Add --merge-method, merge local app components with remote ones by default Add --local-config-method, merge local app components with remote ones by default Oct 9, 2023
@bsquizz bsquizz force-pushed the brandon/local_component_merging branch from 9b38868 to 6c567ed Compare October 13, 2023 13:26
@bsquizz bsquizz merged commit 52517de into master Oct 13, 2023
@bsquizz bsquizz deleted the brandon/local_component_merging branch October 13, 2023 13:30
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.

Support overriding a component via a yaml
3 participants