feat: ArtifactRepositoryRef ConfigMap is now taken from the workflow namespace

[Ark-kun]

Previously, it used the controller's namespace.

[gaoning777]

/lgtm

[alexec]

@jessesuen this PR changes to use the workflow's namespace for the artifact repo. I'm pretty sure this is a breaking change. Can I get your input please?

[alexec]

Looks good, I've asked @jessesuen for some commentary on this change. In the meantime, you might want to consider the following:

1. Sync with master and make sure all checks are green.
2. Consider how you might add tests for this.
   Please dismiss this review once you've made those changes.

[Ark-kun]

This test failure seems to be a fluke. @sarabala1979 is it possible to rerun them?

@jessesuen this PR changes to use the workflow's namespace for the artifact repo. I'm pretty sure this is a breaking change. Can I get your input please?

Hello Alex.

I'm making a non-breaking improvement to a feature I've introduced before: #1350.
Previously, I've added a way to use per-workflow artifact location configmap. This enables different workflows or users to specify different artifact locations without having to hardcode all storage server details (endpoints, ports, secret keys) in the workflow.
Unfortunately, I made a mistake and used the workflow controller's namespace to locate the configmap. This make the following major user scenario impossible: each user has different namespace with their own secrets and artifact location configmaps. This PR fixes this deficiency. This does not break backwards compatibility since the lookup process falls back to the old behavior.

[alexec]

Thank @Ark-kun - am I right in thinking your change was released in v2.4, but that you consider this a bug?

P.s. you should sync with master, as I don't think you have the config for running the Circle CI tests.

[Ark-kun]

Thank @Ark-kun - am I right in thinking your change was released in v2.4, but that you consider this a bug?

We've realized that the the feature failed to cover the biggest user scenario.
The feature released in 2.4.0 covered the scenario:
"Different users use same namespace, but differently named configMaps".
while a very big user scenario is actually
"Different users use different namespaces, with namespaced configMaps".

P.s. you should sync with master, as I don't think you have the config for running the Circle CI tests.

Thank you. Looks like it worked. This baffled me since usually the CI systems test the PR+master merge version.

[codecov]

Codecov Report

Merging #1821 into master will increase coverage by 1.05%.
The diff coverage is 0.97%.

@@            Coverage Diff             @@
##           master    #1821      +/-   ##
==========================================
+ Coverage   10.65%   11.71%   +1.05%     
==========================================
  Files          35       52      +17     
  Lines       24933    26324    +1391     
==========================================
+ Hits         2657     3083     +426     
- Misses      21937    22846     +909     
- Partials      339      395      +56

Impacted Files	Coverage Δ
pkg/apis/workflow/v1alpha1/item.go	55.81% <ø> (ø)	⬆️
...kg/apis/workflow/v1alpha1/zz_generated.deepcopy.go	0% <ø> (ø)	⬆️
...erver/workflowtemplate/workflow_template_server.go	54% <ø> (ø)
server/auth/gatekeeper.go	26.86% <ø> (ø)
.../apis/workflow/v1alpha1/workflow_template_types.go	46.15% <ø> (+46.15%)	⬆️
server/workflowarchive/archived_workflow_server.go	62.5% <ø> (ø)
server/workflow/workflow_server.go	34.7% <ø> (ø)
pkg/apis/workflow/v1alpha1/openapi_generated.go	0% <ø> (ø)	⬆️
pkg/apis/workflow/v1alpha1/generated.pb.go	0.45% <ø> (-0.01%)	⬇️
server/cronworkflow/cron_workflow_server.go	66.66% <ø> (ø)
... and 54 more

---

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 4cf6810...36003d8. Read the comment docs.

[Ark-kun]

@alexec The tests are now passing.

[Ark-kun]

@alexec Gentle ping

[alexec]

Looks good. Lets hold this for v2.6

[sarabala1979]

LGTM nice feature for multi-tendency. As @alexec mentioned, this PR will be targetted on v2.6