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(ui): Change pod names to new format. Fixes #6865 #6925

Merged
merged 7 commits into from
Oct 15, 2021

Conversation

JPZ13
Copy link
Member

@JPZ13 JPZ13 commented Oct 13, 2021

Signed-off-by: J.P. Zivalich j.p.zivalich@gmail.com

This PR:

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #6925 (a163fe0) into master (1bcfa1a) will decrease coverage by 0.01%.
The diff coverage is n/a.

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

@@            Coverage Diff             @@
##           master    #6925      +/-   ##
==========================================
- Coverage   48.56%   48.55%   -0.02%     
==========================================
  Files         265      265              
  Lines       19263    19260       -3     
==========================================
- Hits         9356     9352       -4     
- Misses       8855     8857       +2     
+ Partials     1052     1051       -1     
Impacted Files Coverage Δ
cmd/argo/commands/server.go 29.92% <0.00%> (-1.51%) ⬇️
cmd/argo/commands/get.go 58.89% <0.00%> (-0.88%) ⬇️
cmd/argoexec/commands/emissary.go 51.79% <0.00%> (+1.43%) ⬆️

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 1bcfa1a...3b4cab8. Read the comment docs.

@JPZ13 JPZ13 marked this pull request as ready for review October 14, 2021 21:31

// getPodName returns a deterministic pod name
getPodName(workflowName: string, nodeName: string, templateName: string, nodeID: string): string {
if (process.env.POD_NAMES === 'v1') {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this work when we've baked the image? I don't think there is an env? I'm not sure how we solve the configuration problem without providing an API, and I don't want to provide an API as it's meant to be emergency use only

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'm not sure - I haven't spent any time looking at the UI build process. In the meantime, I removed the env reference

@@ -117,5 +120,46 @@ export const Utils = {
// return a namespace, never return null/undefined, defaults to "default"
getNamespaceWithDefault(namespace: string) {
return this.managedNamespace || namespace || this.currentNamespace || 'default';
},

ensurePodNamePrefixLength(prefix: string): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be tempted to put these into their own file, maybe add tests. I think that work could be done as tech debt.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense

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.

I'm going to approve so we can fix this for users. @JPZ13 I think there is some tech debt here, could I ask you to raise a tech debt issue and we can sort it out later.

@JPZ13
Copy link
Member Author

JPZ13 commented Oct 15, 2021

I'm going to approve so we can fix this for users. @JPZ13 I think there is some tech debt here, could I ask you to raise a tech debt issue and we can sort it out later.

Certainly. I made the tech debt issue at #6946. Feel free to assign it to me, and I can most likely knock it out next week once I return home from traveling

JPZ13 added 7 commits October 15, 2021 09:19
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
@alexec alexec merged commit 67fe87b into argoproj:master Oct 15, 2021
@sarabala1979 sarabala1979 mentioned this pull request Oct 15, 2021
19 tasks
@sarabala1979 sarabala1979 mentioned this pull request Oct 19, 2021
19 tasks
kriti-sc pushed a commit to kriti-sc/argo-workflows that referenced this pull request Oct 24, 2021
…j#6925)

Signed-off-by: kriti-sc <kathuriakriti1@gmail.com>
@alexec alexec mentioned this pull request Nov 5, 2021
25 tasks
@sarabala1979 sarabala1979 mentioned this pull request Dec 15, 2021
73 tasks
@sarabala1979 sarabala1979 mentioned this pull request Mar 1, 2022
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.

Cannot view logs in UI for multi-template workflows
2 participants