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

fix: login/logout using the configured domain #4050

Merged
merged 5 commits into from
Apr 11, 2023
Merged

Conversation

mkhq
Copy link
Contributor

@mkhq mkhq commented Apr 5, 2023

What this PR does / why we need it:

Ensures that the login/logout commands uses the cloud domain in the project config. Both have the noProject flag set which leads to the garden instance not being initialized with a project config. Ensures that this is compatible with garden dev and also includes login/logout in the help listing.

Which issue(s) this PR fixes:

Fixes #4049

Special notes for your reviewer:

@edvald There was an earlier commit which reverted this behavior because of the dev command, 7f93b90. Could you give some more context on how this relates?

@mkhq mkhq requested review from edvald and vvagaytsev April 5, 2023 12:11
// so we initialize it here. noProject also make sure that the project config is not
// initialized in the garden class, so we need to read it in here to get the cloud
// domain.
const projectConfig: ProjectResource | undefined = await cli!.getProjectConfig(log, garden.projectRoot)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There was a reason for changing this. The cli parameter is not populated when running from the dev command. This will need to be fixed differently, with that in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

login and logout are currently not listed as commands in the garden dev help. Should we make that possible?

Copy link
Collaborator

Choose a reason for hiding this comment

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

They should be listed imo. I think it's a reasonable thing to do in the console.

@mkhq mkhq requested a review from edvald April 5, 2023 12:43
@@ -50,6 +50,7 @@ const directInputKeys = [
]

const hideCommands = ["config analytics-enabled", "tools"]
const overrideHiddenCommands = ["login", "logout"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

A simpler approach would be to just remove the hidden flag from the commands :P I don't see any reason not to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is a bit clunky... I looked into where hidden is used and found this for the reference docs: https://github.com/garden-io/garden/blob/0.13/core/src/docs/commands.ts#L26.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I meant specifically for the login/logout commands. The flag is not necessary on them, and I'm not even sure why it's there in the first place.

@edvald
Copy link
Collaborator

edvald commented Apr 5, 2023

One last comment, otherwise seems good to me. Did you test it in the dev command?

@mkhq mkhq force-pushed the 0.13-fix-cloud-domain branch from e6b3cf7 to 788e7a7 Compare April 5, 2023 15:50
@mkhq
Copy link
Contributor Author

mkhq commented Apr 5, 2023

One last comment, otherwise seems good to me. Did you test it in the dev command?

Yes a couple of times :)

@mkhq mkhq added the 0.13 label Apr 5, 2023
@mkhq mkhq requested a review from edvald April 5, 2023 16:23
@vvagaytsev
Copy link
Collaborator

@edvald is this ready to go?

@mkhq mkhq merged commit 1e444e7 into 0.13 Apr 11, 2023
@mkhq mkhq deleted the 0.13-fix-cloud-domain branch April 11, 2023 19:14
This was referenced May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants