Skip to content

Rename 'setup' to 'setup_app_templates' in config file #112

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
Merged
merged 10 commits into from
Dec 1, 2023
Merged

Rename 'setup' to 'setup_app_templates' in config file #112

merged 10 commits into from
Dec 1, 2023

Conversation

ahangarha
Copy link
Contributor

@ahangarha ahangarha commented Nov 20, 2023 •
edited
Loading

Note #96 (comment)

I have two proposals here:

  1. Drop the setup-app command entirely and replace it with --all in apply-template.
    I think the value of this command is to ease the initial setup of an app with all of its templates. If the user has any specific use case, then the normal usage of apply-template t1 t2 t3 is there.
  2. We assume setup_app_template includes all templates unless specified in the config file. This means if the user has not made an entry in the config file, still setup-app can be used, and it applies all the templates. In this case, the user can white-list templates in the config file under setup_app_template.

I think the first approach is better because it is much simpler.

Closes #96

@ahangarha ahangarha mentioned this pull request Nov 20, 2023
Closed
rafaelgomesxyz
rafaelgomesxyz requested changes Nov 23, 2023
View reviewed changes
justin808
justin808 requested changes Nov 25, 2023
View reviewed changes
Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Need a deprecation message and CHANGELOG entry.

@justin808 justin808 marked this pull request as draft November 25, 2023 03:37
ahangarha
ahangarha commented Nov 25, 2023
View reviewed changes
lib/core/config.rb
@@ -133,6 +133,7 @@ def new_option_keys
org: :cpln_org,
location: :default_location,
prefix: :match_if_app_name_starts_with,
setup: :setup_app_templates,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@justin808
We get the intended deprecation message:

DEPRECATED: Option 'setup' is deprecated, please use 'setup_app_templates' instead (in 'controlplane.yml').

@ahangarha ahangarha marked this pull request as ready for review November 25, 2023 10:35
@ahangarha ahangarha mentioned this pull request Nov 25, 2023
Open
17 tasks
rafaelgomesxyz
rafaelgomesxyz approved these changes Nov 25, 2023
View reviewed changes
Copy link
Collaborator

@rafaelgomesxyz rafaelgomesxyz left a comment

Choose a reason for hiding this comment

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

Just a typo in the changelog, the rest looks good to me.

@justin808 justin808 merged commit ab7b8d3 into main Dec 1, 2023
@justin808 justin808 deleted the rename-setup-to-setup-app-templates-in-config-file branch December 1, 2023 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafaelgomesxyz rafaelgomesxyz rafaelgomesxyz approved these changes

@justin808 justin808 Awaiting requested review from justin808

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

Successfully merging this pull request may close these issues.

Rename controlplane.yml key setup to setup_app_workloads for clarity
3 participants
@ahangarha @justin808 @rafaelgomesxyz