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.

Sign up for GitHub

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

Jump to bottom

Allow to select the namespace on a dashboard #1291

Merged
merged 9 commits into from
Jun 24, 2021
Merged

Allow to select the namespace on a dashboard #1291

merged 9 commits into from
Jun 24, 2021

Conversation

perenesenko
Copy link
Member

@perenesenko perenesenko commented Jun 17, 2021 •
edited
Loading

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@perenesenko
Copy link
Member Author

PR for the task #1255

@codecov
Copy link

codecov bot commented Jun 17, 2021 •
edited
Loading

Codecov Report

Merging #1291 (a54110e) into master (96ba030) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head a54110e differs from pull request most recent head b7d2491. Consider uploading reports for the commit b7d2491 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1291      +/-   ##
==========================================
+ Coverage   81.41%   81.44%   +0.03%     
==========================================
  Files         106      106              
  Lines        9702     9702              
==========================================
+ Hits         7899     7902       +3     
+ Misses       1266     1264       -2     
+ Partials      537      536       -1
Impacted Files Coverage Δ
rollout/trafficrouting/istio/controller.go 45.73% <0.00%> (+2.32%) ⬆️

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 96ba030...b7d2491. Read the comment docs.

@perenesenko perenesenko mentioned this pull request Jun 17, 2021
Closed
@perenesenko perenesenko force-pushed the perenesenko/argo-rollouts-1255 branch from 1304d4d to fc7d8d9 Compare June 17, 2021 19:49
@rbreeze
Copy link
Member

rbreeze commented Jun 17, 2021

Could you include a screenshot here of the namespace picker?

@rbreeze rbreeze self-requested a review June 17, 2021 20:22
jessesuen
jessesuen reviewed Jun 17, 2021
View reviewed changes
examples/dashboard.json Outdated Show resolved Hide resolved
jessesuen
jessesuen reviewed Jun 17, 2021
View reviewed changes
examples/dashboard.json Outdated Show resolved Hide resolved
alexmt
alexmt reviewed Jun 17, 2021
View reviewed changes
server/server.go Outdated Show resolved Hide resolved
@perenesenko
Copy link
Member Author

perenesenko commented Jun 21, 2021 •
edited
Loading

screenshot for the namespace picker
image

@perenesenko perenesenko changed the title Allow user to select namespace on argo-rollouts dashboard Allow to select the namespace on a dashboard Jun 21, 2021
huikang
huikang reviewed Jun 21, 2021
View reviewed changes
server/server.go
@@ -258,20 +227,27 @@ func (s *ArgoRolloutsServer) WatchRolloutInfo(q *rollout.RolloutInfoQuery, ws ro
return nil
}

func (s *ArgoRolloutsServer) ListReplicaSetsAndPods(ctx context.Context) ([]*appsv1.ReplicaSet, []*corev1.Pod, error) {
s.NamespaceVC.Start(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like NamespaceVC can be removed from the type ArgoRolloutsServer struct since we use the KubeClientset.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's removed from ArgoRolloutsServer. I guess you're looking at old file

Copy link
Member

Choose a reason for hiding this comment

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

Hi, @perenesenko , sorry, I forgot to refer to the field in the struct

argo-rollouts/server/server.go

Line 73 in 96ba030

NamespaceVC NamespaceViewController

With the change in this PR, NamespaceVC seems being used in nowhere, so it could be deleted from the struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

@huikang see this message: https://github.com/argoproj/argo-rollouts/pull/1291/files#r655771666

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Thanks

huikang
huikang approved these changes Jun 21, 2021
View reviewed changes
Copy link
Member

@huikang huikang left a comment

Choose a reason for hiding this comment

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

LGTM

@perenesenko perenesenko force-pushed the perenesenko/argo-rollouts-1255 branch from f63499c to 59b20b0 Compare June 22, 2021 17:14
perenesenko and others added 9 commits June 23, 2021 11:11
@perenesenko
1255 Rollout Dashboard should allow user select namespace
526ad78 
Signed-off-by: Andrii Perenesenko <[email protected]>
@perenesenko
feat: Rollout Dashboard should allow user select namespace.
a121cb8 
Signed-off-by: Andrii Perenesenko <[email protected]>
@perenesenko
feat: Rollout Dashboard should allow user select namespace. removing …
95fb474 
…updates in loop

Signed-off-by: Andrii Perenesenko <[email protected]>
@alexmt @perenesenko
move namespace + available namespaces state to app.tsx
4b0fb2d 
Signed-off-by: Alexander Matyushentsev <[email protected]>
Signed-off-by: Andrii Perenesenko <[email protected]>
@perenesenko
feat: Rollout Dashboard should allow user select namespace. Pick valu…
0645071 
…e from the dropdown or from edit box

Signed-off-by: Andrii Perenesenko <[email protected]>
@perenesenko
feat: Rollout Dashboard should allow user select namespace. Get names…
5d4c3db 
…paces list from the rollouts

Signed-off-by: Andrii Perenesenko <[email protected]>
@perenesenko
feat: Rollout Dashboard should allow user select namespace. Return em…
c5828e5 
…pty availableNamespaces if failed to fetch rollouts

Signed-off-by: Andrii Perenesenko <[email protected]>
@perenesenko
fix: Fix the terminating of watch process
f7d6563 
Signed-off-by: Andrii Perenesenko <[email protected]>
@perenesenko
feat: Rollout Dashboard should allow user select namespace. Put the l…
b7d2491 
…abel for NS list

Signed-off-by: Andrii Perenesenko <[email protected]>
@perenesenko perenesenko force-pushed the perenesenko/argo-rollouts-1255 branch from a54110e to b7d2491 Compare June 23, 2021 18:14
@sonarcloud
Copy link

sonarcloud bot commented Jun 23, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
5.5% 5.5% Duplication

@jessesuen jessesuen merged commit 6900988 into argoproj:master Jun 24, 2021
@gempesaw gempesaw mentioned this pull request Jul 9, 2021
Closed
@perenesenko perenesenko deleted the perenesenko/argo-rollouts-1255 branch August 10, 2021 05:17
@bpoland bpoland mentioned this pull request Aug 27, 2021
Closed
@perenesenko perenesenko linked an issue Jan 4, 2022 that may be closed by this pull request
Rollout Dashboard should allow user select namespace #1255
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexmt alexmt alexmt left review comments

@jessesuen jessesuen jessesuen approved these changes

@huikang huikang huikang approved these changes

@rbreeze rbreeze Awaiting requested review from rbreeze

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rollout Dashboard should allow user select namespace
5 participants
@perenesenko @rbreeze @alexmt @huikang @jessesuen