-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix job_run to show appropriate namespace in web UI link #26738
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Have you signed the CLA already but the status is still pending? Recheck it. |
jobNamespace := c.Meta.namespace | ||
if jobNamespace == "" { | ||
jobNamespace = "default" | ||
jobNamespace := "default" | ||
if job.Namespace != nil && *job.Namespace != "" { | ||
jobNamespace = *job.Namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heya @nipunn1313, thanks for the PR!
I believe this resolves the issue for jobs that have namespace
defined within the job specification, but it's also possible to specify the namespace on the CLI:
nomad job run -namespace='some-other-ns' job.nomad.hcl
which is what c.Meta.namespace
reflects, so I think this change breaks that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - I'm realizing we probably want to prioritize the CLI and then if CLI is missing - use the namespace within the job specification.
|
||
// TestRunCommand_NamespaceInUILink tests that the UI link uses the job's namespace | ||
// rather than the CLI namespace when they differ. | ||
func TestRunCommand_NamespaceInUILink(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is actually writing to nomad - I don't think this is what we want - going to remove it.
Description
Fix the UI link to show the appropriate namespace
Testing & Reproduction steps
nomad run
something not in the default namespaceLinks
Contributor Checklist
changelog entry using the
make cl
command.ensure regressions will be caught.
and job configuration, please update the Nomad website documentation to reflect this. Refer to
the website README for docs guidelines. Please also consider whether the
change requires notes within the upgrade guide.
Reviewer Checklist
backporting document.
in the majority of situations. The main exceptions are long-lived feature branches or merges where
history should be preserved.
within the public repository.
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.