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

Update cluster cmd #1487

Merged
merged 3 commits into from Feb 24, 2022
Merged

Update cluster cmd #1487

merged 3 commits into from Feb 24, 2022

Conversation

J-Thompson12
Copy link
Contributor

What changed?
Removed the auth flow from cluster add/delete cmd's. Core is removing git auth but enterprise still needs a git token to create PR's. For now I have left in getting the token from the env variable. I also left in all the code for gitProvider. It isnt needed for add/delete but profiles is using it and that is a problem for another task.

Why?

How did you test it?

Release notes

Documentation Changes

@J-Thompson12 J-Thompson12 changed the base branch from main to v2 February 18, 2022 18:20
}

token = generatedToken
return "", fmt.Errorf(missingTokenErr, tokenVarName)
} else if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this redundant because we've already checked for err above? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above just gets the env var name. So GITHUB vs GITLAB. The second one checks if that var is empty or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

But lookupEnvFunc doesn't set err, so this case is still pointless, right? It was redundant before too, though.

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 yeah I looked at the wrong err. Yeah this is redundant.

Copy link
Contributor

@ozamosi ozamosi left a comment

Choose a reason for hiding this comment

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

If pesto is happy, I'm happy.

@J-Thompson12 J-Thompson12 merged commit 918ba04 into v2 Feb 24, 2022
jpellizzari pushed a commit that referenced this pull request Feb 25, 2022
* Remove auth flow from cluster add/delete

* fix lint error

* remove redundant code

Co-authored-by: Justin Thompson <jpthomp12@gmail.com>
jpellizzari pushed a commit that referenced this pull request Mar 3, 2022
* Remove auth flow from cluster add/delete

* fix lint error

* remove redundant code

Co-authored-by: Justin Thompson <jpthomp12@gmail.com>
@ozamosi ozamosi deleted the update-cluster-cmd branch May 12, 2022 17:42
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.

None yet

3 participants