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

Added logout ability (argocd logout) #1582

Merged
merged 9 commits into from
Jun 7, 2019
Merged

Conversation

simster7
Copy link
Member

@simster7 simster7 commented May 7, 2019

Implements #1210. Pretty self explanatory. Current behavior if logging out from a currently selected context is to automatically select the first context that is still logged in, not sure if this is the desired behavior.

@codecov-io
Copy link

codecov-io commented May 7, 2019

Codecov Report

Merging #1582 into master will increase coverage by 0.01%.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1582      +/-   ##
=========================================
+ Coverage   34.08%   34.1%   +0.01%     
=========================================
  Files          76      77       +1     
  Lines       11354   11413      +59     
=========================================
+ Hits         3870    3892      +22     
- Misses       6951    6983      +32     
- Partials      533     538       +5
Impacted Files Coverage Δ
cmd/argocd/commands/root.go 5.55% <0%> (-0.16%) ⬇️
cmd/argocd/commands/context.go 15.29% <34.21%> (+15.29%) ⬆️
cmd/argocd/commands/logout.go 62.5% <62.5%> (ø)
controller/appcontroller.go 38.62% <0%> (-1.41%) ⬇️
server/server.go 46.74% <0%> (+0.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0088955...0f538da. Read the comment docs.

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

Looks good, but could this have some tests please?

@simster7
Copy link
Member Author

simster7 commented May 8, 2019

@alexec Added tests!

@alexec
Copy link
Contributor

alexec commented May 8, 2019

@alexmt or @jessesuen is this something you'd like to review? I don't know the code.

@jessesuen
Copy link
Member

I took a quick look. This PR is actually implementing context deletion, which is actually different (but useful) request. argocd logout should leave the contexts in place, but simply delete token information. For context management, we can put this under the argocd ctx set of commands. For example, this work would probably make sense as:

argocd ctx --delete

Which will delete the current context.

@simster7
Copy link
Member Author

simster7 commented Jun 4, 2019

@jessesuen I have now included an argocd context --delete flag that will delete the current context and have modified the argocd logout CONTEXT command to only delete the relevant auth-token and refresh-token. Sorry for the delay.

Also, the failing E2E test seems to be a flaky test related to GitLab authentication.

Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

Conditional approval. Please update dependencies to remove changes to openapi_generated.go


const testConfigFilePath = "./testdata/config"

func TestContextDelete(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this

@@ -105,7 +105,6 @@ func schema_pkg_apis_application_v1alpha1_AWSAuthConfig(ref common.ReferenceCall
},
},
},
Dependencies: []string{},
Copy link
Member

Choose a reason for hiding this comment

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

These openapi changes are unexpected. Can you run:

brew upgrade protobuf
brew upgrade go-swagger
make codegen

@simster7
Copy link
Member Author

simster7 commented Jun 7, 2019

@jessesuen Ready!

@alexec alexec merged commit 4860f2c into argoproj:master Jun 7, 2019
@alexec alexec modified the milestone: v1.1 Jun 7, 2019
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.

4 participants