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

fix: Match cli display pod names with k8s. Fixes #7646 #7653

Merged
merged 5 commits into from
Jan 27, 2022
Merged

fix: Match cli display pod names with k8s. Fixes #7646 #7653

merged 5 commits into from
Jan 27, 2022

Conversation

JPZ13
Copy link
Member

@JPZ13 JPZ13 commented Jan 26, 2022 •
edited
Loading

This PR:

Name:                dag-diamond-xtpd7
Namespace:           argo
ServiceAccount:      unset (will run with the default ServiceAccount)
Status:              Succeeded
Conditions:
 PodRunning          False
 Completed           True
Created:             Wed Jan 26 16:32:00 -0500 (13 seconds ago)
Started:             Wed Jan 26 16:32:00 -0500 (13 seconds ago)
Finished:            Wed Jan 26 16:32:13 -0500 (now)
Duration:            13 seconds
Progress:            4/4
ResourcesDuration:   8s*(1 cpu),0s*(100Mi memory)

STEP                  TEMPLATE  PODNAME                            DURATION  MESSAGE
 ✔ dag-diamond-xtpd7  diamond
 ├─✔ A                echo      dag-diamond-xtpd7-echo-3871903043  3s
 ├─✔ B                echo      dag-diamond-xtpd7-echo-3888680662  3s
 ├─✔ C                echo      dag-diamond-xtpd7-echo-3905458281  3s
 └─✔ D                echo      dag-diamond-xtpd7-echo-3922235900  3s

Signed-off-by: J.P. Zivalich jp@pipekit.io

@JPZ13
fix: Match cli display pod names with k8s. Fixes argoproj#7646
adb9d55 
Signed-off-by: J.P. Zivalich <jp@pipekit.io>
@JPZ13
fix: Remove stray console statement
81aec49 
Signed-off-by: J.P. Zivalich <jp@pipekit.io>
@JPZ13
fix: Factor out pod name version in pod names fn
63d34b6 
Signed-off-by: J.P. Zivalich <jp@pipekit.io>
@JPZ13
refactor: Use GetPodNameVersion in tests
530d223 
Signed-off-by: J.P. Zivalich <jp@pipekit.io>
@JPZ13 JPZ13 marked this pull request as ready for review January 27, 2022 00:17
@JPZ13 JPZ13 marked this pull request as draft January 27, 2022 00:17
@JPZ13 JPZ13 marked this pull request as ready for review January 27, 2022 00:37
terrytangyuan
terrytangyuan reviewed Jan 27, 2022
View reviewed changes
test/e2e/fixtures/then.go Outdated
@@ -86,9 +87,15 @@ func (t *Then) ExpectWorkflowNode(selector func(status wfv1.NodeStatus) bool, f
if n != nil {
_, _ = fmt.Println("Found node", "id="+n.ID, "type="+n.Type)
if n.Type == wfv1.NodeTypePod {
version := util.PodNameV1
annotations := metadata.GetAnnotations()
if annotations[common.AnnotationKeyPodNameVersion] == util.PodNameV2.String() {
Copy link
Member

Choose a reason for hiding this comment

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

Reuse GetWorkflowPodNameVersion?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about it but didn't have a workflow object. Would you recommend building one using the metadata variable, or just leaving as is?

Copy link
Member

Choose a reason for hiding this comment

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

I think creating a workflow object would be better to reduce code redundancy and make it easier later when we start removing v1 pod name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I'll make the switch

@JPZ13
refactor: Use wf object in test fixture
6416884 
Signed-off-by: J.P. Zivalich <jp@pipekit.io>
terrytangyuan
terrytangyuan approved these changes Jan 27, 2022
View reviewed changes
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Thanks!

@terrytangyuan terrytangyuan merged commit 1159afc into argoproj:master Jan 27, 2022
@alexec alexec mentioned this pull request Jan 27, 2022
Closed
4 tasks
alexec pushed a commit that referenced this pull request Jan 27, 2022
@JPZ13 @alexec
fix: Match cli display pod names with k8s. Fixes #7646 (#7653)
342e44a 
* fix: Match cli display pod names with k8s. Fixes #7646

Signed-off-by: J.P. Zivalich <jp@pipekit.io>

* fix: Remove stray console statement

Signed-off-by: J.P. Zivalich <jp@pipekit.io>

* fix: Factor out pod name version in pod names fn

Signed-off-by: J.P. Zivalich <jp@pipekit.io>

* refactor: Use GetPodNameVersion in tests

Signed-off-by: J.P. Zivalich <jp@pipekit.io>

* refactor: Use wf object in test fixture

Signed-off-by: J.P. Zivalich <jp@pipekit.io>
@JPZ13 JPZ13 mentioned this pull request Feb 17, 2022
Closed
5 tasks
@sarabala1979 sarabala1979 mentioned this pull request Mar 1, 2022
Closed
@crenshaw-dev crenshaw-dev mentioned this pull request Nov 2, 2023
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terrytangyuan terrytangyuan terrytangyuan approved these changes

Assignees

@terrytangyuan terrytangyuan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PODNAMES as shown from argocli does not match with actual generated pod names
2 participants
@JPZ13 @terrytangyuan