Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixup: Rename -application-name to just -app for consistency #77

Merged
merged 3 commits into from
May 23, 2024

Conversation

briancain
Copy link
Member

@briancain briancain commented May 15, 2024

Changes proposed in this PR:

Other HCP commands use -app for the app name, and it's much less typing than -application-name. This simplifies this flag value.

How I've tested this PR:

Built and validated the flag name changed, updated the tests and saw they passed.

How I expect reviewers to test this PR:

Same ☝🏻

Checklist:

  • Tests added if applicable
  • CHANGELOG entry added or label 'pr/no-changelog' added to PR

    Run CHANGELOG_PR=<PR number> make changelog/new-entry for guidance
    in authoring a changelog entry, and commit the resulting file, which should
    have a name matching your PR number. Entries should use imperative present
    tense (e.g. Add support for...)

Other HCP commands use -app for the app name, and it's much less typing
than -application-name. This simplifies this flag value.
@briancain briancain requested a review from paladin-devops May 15, 2024 21:38
@briancain briancain requested a review from a team as a code owner May 15, 2024 21:38
internal/commands/waypoint/add-ons/create.go Outdated Show resolved Hide resolved
@@ -58,8 +58,8 @@ $ hcp waypoint add-ons create -n=my-addon -a=my-application -d=my-addon-definiti
Required: true,
},
{
Name: "application-name",
DisplayValue: "NAME",
Name: "app",
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder since this is a breaking change, how about we add both into the mix for some period of time, and then let users drift away. Not sure if it is possible, but when specified both could update the same underlying var.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't feel like that's worth the maintenance effort. Maybe if this CLI had been around for years sure, but at this point it's brand new (not to mention Waypoint is still in beta) so I'd rather make the breaking change than maintain two flags for backwards compatibility right now. I do appreciate the suggestion though!

Copy link
Contributor

@codergs codergs left a comment

Choose a reason for hiding this comment

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

LGTM. Please fill the description section if possible 👍

Copy link
Contributor

@paladin-devops paladin-devops left a comment

Choose a reason for hiding this comment

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

The add-on-appening! ✅

@briancain briancain merged commit 8d9e809 into main May 23, 2024
6 checks passed
@briancain briancain deleted the waypoint/addons/standardize-app-flag branch May 23, 2024 21:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants