Skip to content
This repository was archived by the owner on Jan 8, 2024. It is now read-only.

Fix for empty deployUrl check in deployment_create #2755

Merged
merged 6 commits into from
Nov 30, 2021

Conversation

gunadhya
Copy link
Contributor

Fixes #2645

I'm new to this project so please let me know if any updates are needed.

@github-actions github-actions bot added the core label Nov 19, 2021
app.UI.Output(" Release URL: %s", releaseUrl, terminal.WithSuccessStyle())
app.UI.Output("Deployment URL: https://%s", deployUrl, terminal.WithSuccessStyle())

if deployUrl != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for making the PR @gunadhya and you're on the right track! We already have a template defined when URL service is disabled, I would suggest refactoring the case switch statement to hit that code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Do you mean a new case like this?

    case releaseUrl != "" && deployUrl == "":
	app.UI.Output(strings.TrimSpace(deployNoURL)+"\n", terminal.WithSuccessStyle())
	app.UI.Output("   Release URL: %s", releaseUrl, terminal.WithSuccessStyle())

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant a case deployUrl == "" that wouldn't print out the release URL, but thinking more about it we do want the release URL too, so an if statement inside the releaseUrl case was the proper approach, I'd do something like

case releaseUrl != "":
    printInplaceInfo(inplace,app)
    app.UI.Output(" Release URL: %s", releaseUrl, terminal.WithSuccessStyle())
    if deployUrl != "" {
        app.UI.Output("Deployment URL: https://%s", deployUrl 
    } else {
        app.UI.Output(strings.TrimSpace(deployNoURL)+"\n", terminal.WithSuccessStyle())
    }

Copy link
Contributor

@xiaolin-ninja xiaolin-ninja left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@briancain briancain left a comment

Choose a reason for hiding this comment

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

Hey there @gunadhya - could you please add a changelog? Our guide on how to do this is here: https://github.com/hashicorp/waypoint/blob/main/.github/CHANGELOG_GUIDE.md

This is a bug fix type

@gunadhya
Copy link
Contributor Author

I've added a changelog. If there are any other beginner/Intermediate issues in this project, I'd be happy to work on them.

Copy link
Member

@briancain briancain left a comment

Choose a reason for hiding this comment

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

Hey @gunadhya - The changelog file name should match this pull request, so it should be updated to

.changelog/2755.txt

Thanks!

Otherwise I think we're good to merge.

@gunadhya
Copy link
Contributor Author

Oh I'll get right on it. Does the documentation needed to be updated https://github.com/hashicorp/waypoint/blob/main/.github/CHANGELOG_GUIDE.md#changelog-entry-examples
Cause over there it suggested issue number.

@briancain
Copy link
Member

Oops, yep, that should be updated! Sorry about that ❤️

@briancain
Copy link
Member

I've added a changelog. If there are any other beginner/Intermediate issues in this project, I'd be happy to work on them.

Hmm, will have to get back to you on that one! If you see something interesting in our backlog of issues, please feel free to drop a comment on it to discuss and we can figure something out before you get started on an issue 😄

@briancain briancain merged commit cd116ea into hashicorp:main Nov 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

waypoint deploy's logs reference horizon features even with the url service disabled
3 participants