chore!: remove duplicate Server env vars, plus `--basehref` -> `--base-href` #12653

agilgur5 · 2024-02-11T00:50:38Z

Follow-up to #12652 and #6923 (comment) on findings of duplicate env vars

Motivation

every Server CLI flag has had an equivalent env var since feat: Add env vars config for argo-server and workflow-controller #6767
- ALLOWED_LINK_PROTOCOL and BASE_HREF each had a duplicate env var as the odd ones out
The Server flag --basehref was also inconsistent with the env var, other flags, and the client, change it to --base-href for consistency

Modifications

remove the two env vars in a breaking change, and refer users to their ARGO_* equivalents in the upgrade notes
replace --basehref with --base-href as a breaking change and mention it in the upgrade notes
modify the docs and descriptions that referred to the two env vars
- plus some small grammar and consistency fixes therein

Verification

Docs build & lint pass.

Notes to Reviewers

Can skip docs/cli/* as it's all auto-generated from the CLI changes

- every CLI flag has had an equivalent env var since 96c5626 - `ALLOWED_LINK_PROTOCOL` and `BASE_HREF` each had a duplicate env var as the odd ones out - so remove them in a breaking change, and refer users to their `ARGO_*` equivalents in the upgrade notes Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

tczhao

I think we also need to change
https://github.com/argoproj/argo-workflows/blob/main/cmd/argo/commands/root.go#L87C102-L87C111
to ARGO_BASE_HREF

cmd/argo/commands/server.go

agilgur5 · 2024-02-13T06:06:11Z

I think we also need to change https://github.com/argoproj/argo-workflows/blob/main/cmd/argo/commands/root.go#L87C102-L87C111 to ARGO_BASE_HREF

~~See my in-line comment, I gave context on this in the PR description.~~ EDIT: per below, I misread, this is a separate Long description/comment of the CLI, my bad!

tczhao · 2024-02-14T04:20:11Z

Thanks for the links. They make a lot of sense!
I still think we need to change https://github.com/argoproj/argo-workflows/blob/main/cmd/argo/commands/root.go#L87C102-L87C111
from
If your server is behind an ingress with a path (you'll be running "argo server --basehref /...) or "BASE_HREF=/... argo server"):
to
If your server is behind an ingress with a path (you'll be running "argo server --basehref /...) or "ARGO_BASE_HREF=/... argo server"):
or in any style that make sense, the BASE_HREF=/... argo server seems wrong since the env var now has a ARGO_ preifx

agilgur5 · 2024-02-14T06:09:24Z

Ohhh, good catch! I totally missed that in the root.go, especially since it says ARGO_BASE_HREF on the very next line. Sorry I misread your first comment! Thanks for clarifying and quoting it directly, that made it more obvious 🙂

Yea let me modify that too. I could've sworn I searched for all references to BASE_HREF so let me double check that too if I missed this one

agilgur5 · 2024-02-14T06:15:05Z

https://github.com/argoproj/argo-workflows/blob/main/cmd/argo/commands/root.go#L87C102-L87C111

Ah I totally missed this column number in the link -- I was on mobile where it's far off screen to the right so I didn't see the column highlight on first glance

(Screenshot for reference: )

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

agilgur5 · 2024-02-14T18:23:23Z

https://github.com/argoproj/argo-workflows/blob/main/cmd/argo/commands/root.go#L87C102-L87C111

Yea let me modify that too. I could've sworn I searched for all references to BASE_HREF so let me double check that too if I missed this one

Fixed this one plus a few others. That should be all of them now

agilgur5 · 2024-02-14T18:24:23Z

cmd/argo/commands/client/conn.go

@@ -45,7 +45,7 @@ func AddAPIClientFlagsToCmd(cmd *cobra.Command) {
 	cmd.PersistentFlags().StringVar(&instanceID, "instanceid", os.Getenv("ARGO_INSTANCEID"), "submit with a specific controller's instance id label. Default to the ARGO_INSTANCEID environment variable.")
 	// "-s" like kubectl
 	cmd.PersistentFlags().StringVarP(&ArgoServerOpts.URL, "argo-server", "s", os.Getenv("ARGO_SERVER"), "API server `host:port`. e.g. localhost:2746. Defaults to the ARGO_SERVER environment variable.")
-	cmd.PersistentFlags().StringVar(&ArgoServerOpts.Path, "argo-base-href", os.Getenv("ARGO_BASE_HREF"), "Path to use with HTTP client due to BASE_HREF. Defaults to the ARGO_BASE_HREF environment variable.")
+	cmd.PersistentFlags().StringVar(&ArgoServerOpts.Path, "argo-base-href", os.Getenv("ARGO_BASE_HREF"), "Path to use with HTTP client due to Base HREF. Defaults to the ARGO_BASE_HREF environment variable.")


this is referring to the "Base HREF" feature now and not the specific env var

tczhao · 2024-02-14T21:57:32Z

https://github.com/argoproj/argo-workflows/blob/main/cmd/argo/commands/root.go#L87C102-L87C111

Ah I totally missed this column number in the link -- I was on mobile where it's far off screen to the right so I didn't see the column highlight on first glance

(Screenshot for reference: )

No worries. It happens

tczhao

LGTM

cmd/argo/commands/server.go

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

sarabala1979

LGTM
Before approve, @terrytangyuan @juliev0 can you take a quick look because of it is breaking change?

rijos · 2024-02-28T11:44:18Z

@agilgur5, seems this change will break the helm chart, it still refers to BASE_HREF. See: https://github.com/argoproj/argo-helm/blob/2f82fb5992fe1e390d1ebdbc4be6d5d6c6549a37/charts/argo-workflows/templates/server/server-deployment.yaml#L98

agilgur5 · 2024-02-28T14:27:20Z

Yes, all downstream consumers that use this, like the Helm Charts, will need to be updated. That is the nature of breaking changes.

agilgur5 · 2024-05-22T19:26:36Z

Merged main for new PR checks per #13027

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

agilgur5 · 2024-06-09T18:31:59Z

Going to merge this as Tianchu approved and Bala LGTM'd and there have been no reviews from others in the past ~4 months

agilgur5 · 2024-06-09T18:43:09Z

@agilgur5, seems this change will break the helm chart, it still refers to BASE_HREF. See: https://github.com/argoproj/argo-helm/blob/2f82fb5992fe1e390d1ebdbc4be6d5d6c6549a37/charts/argo-workflows/templates/server/server-deployment.yaml#L98

Since ARGO_BASE_HREF already exists, I addressed this in the chart for forward compat: argoproj/argo-helm#2756

agilgur5 · 2024-06-17T17:58:23Z

Since ARGO_BASE_HREF already exists, I addressed this in the chart for forward compat: argoproj/argo-helm#2756

Annnd I didn't re-read my own release notes or this PR (it's 4 months old, so quite a time delay) and caused a regression downstream in Argo Helm. Reverted that change in argoproj/argo-helm#2770
Sorry I screwed that up, I meant to be proactive there, but the time delay meant I forgot how my own change worked

agilgur5 added area/docs Incorrect, missing, or mistakes in docs area/server labels Feb 11, 2024

agilgur5 mentioned this pull request Feb 11, 2024

fix(build): check for env vars in all dirs #12652

Merged

agilgur5 added the type/tech-debt label Feb 11, 2024

agilgur5 force-pushed the chore-server-remove-dupe-envs branch from b0c047a to 8571e1f Compare February 11, 2024 21:37

agilgur5 marked this pull request as ready for review February 11, 2024 21:39

tczhao requested changes Feb 13, 2024

View reviewed changes

cmd/argo/commands/server.go Show resolved Hide resolved

agilgur5 added 2 commits February 14, 2024 13:18

fix remaining base_href references

f3cfcaf

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

cli docgen

b8e7154

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

agilgur5 commented Feb 14, 2024

View reviewed changes

agilgur5 requested a review from tczhao February 14, 2024 18:27

tczhao approved these changes Feb 14, 2024

View reviewed changes

agilgur5 requested a review from terrytangyuan February 14, 2024 22:05

sarabala1979 reviewed Feb 19, 2024

View reviewed changes

cmd/argo/commands/server.go Show resolved Hide resolved

agilgur5 added 2 commits February 19, 2024 11:12

fix basehref -> base-href, add another breaking change note

2b34c7f

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

codegen CLI docs

2d35e3e

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

agilgur5 changed the title ~~chore!: remove duplicated Server env vars~~ chore!: remove duplicate Server env vars, plus --basehref -> --base-href Feb 19, 2024

agilgur5 requested a review from sarabala1979 February 19, 2024 16:21

sarabala1979 reviewed Feb 20, 2024

View reviewed changes

agilgur5 added this to the v3.6.0 milestone Apr 12, 2024

Merge branch 'main' into chore-server-remove-dupe-envs

dcece75

fix codegen

0eb9c08

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

agilgur5 enabled auto-merge (squash) June 9, 2024 18:33

agilgur5 mentioned this pull request Jun 9, 2024

fix(argo-workflows): BASE_HREF -> ARGO_BASE_HREF for forward compat argoproj/argo-helm#2756

Merged

4 tasks

agilgur5 merged commit b560ad3 into argoproj:main Jun 9, 2024
29 checks passed

agilgur5 deleted the chore-server-remove-dupe-envs branch June 9, 2024 18:57

This was referenced Jun 17, 2024

Regression: ARGO_BASE_HREF env var doesn't work #13199

Closed

fix(argo-workflows): revert BASE_HREF change argoproj/argo-helm#2770

Merged

agilgur5 mentioned this pull request Jun 21, 2024

fix(templating)!: return json, not go structs #12909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore!: remove duplicate Server env vars, plus `--basehref` -> `--base-href` #12653

chore!: remove duplicate Server env vars, plus `--basehref` -> `--base-href` #12653

agilgur5 commented Feb 11, 2024 •

edited

Loading

tczhao left a comment

agilgur5 commented Feb 13, 2024 •

edited

Loading

tczhao commented Feb 14, 2024

agilgur5 commented Feb 14, 2024 •

edited

Loading

agilgur5 commented Feb 14, 2024 •

edited

Loading

agilgur5 commented Feb 14, 2024

agilgur5 Feb 14, 2024

tczhao commented Feb 14, 2024

tczhao left a comment

sarabala1979 left a comment

rijos commented Feb 28, 2024

agilgur5 commented Feb 28, 2024

agilgur5 commented May 22, 2024 •

edited

Loading

agilgur5 commented Jun 9, 2024

agilgur5 commented Jun 9, 2024

agilgur5 commented Jun 17, 2024

chore!: remove duplicate Server env vars, plus --basehref -> --base-href #12653

chore!: remove duplicate Server env vars, plus --basehref -> --base-href #12653

Conversation

agilgur5 commented Feb 11, 2024 • edited Loading

Motivation

Modifications

Verification

Notes to Reviewers

tczhao left a comment

Choose a reason for hiding this comment

agilgur5 commented Feb 13, 2024 • edited Loading

tczhao commented Feb 14, 2024

agilgur5 commented Feb 14, 2024 • edited Loading

agilgur5 commented Feb 14, 2024 • edited Loading

agilgur5 commented Feb 14, 2024

agilgur5 Feb 14, 2024

Choose a reason for hiding this comment

tczhao commented Feb 14, 2024

tczhao left a comment

Choose a reason for hiding this comment

sarabala1979 left a comment

Choose a reason for hiding this comment

rijos commented Feb 28, 2024

agilgur5 commented Feb 28, 2024

agilgur5 commented May 22, 2024 • edited Loading

agilgur5 commented Jun 9, 2024

agilgur5 commented Jun 9, 2024

agilgur5 commented Jun 17, 2024

chore!: remove duplicate Server env vars, plus `--basehref` -> `--base-href` #12653

chore!: remove duplicate Server env vars, plus `--basehref` -> `--base-href` #12653

agilgur5 commented Feb 11, 2024 •

edited

Loading

agilgur5 commented Feb 13, 2024 •

edited

Loading

agilgur5 commented Feb 14, 2024 •

edited

Loading

agilgur5 commented Feb 14, 2024 •

edited

Loading

agilgur5 commented May 22, 2024 •

edited

Loading