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(controller/cli): Resolve global artifacts created in nested workflows #6422

Merged
merged 5 commits into from
Jul 27, 2021

Conversation

changhc
Copy link
Contributor

@changhc changhc commented Jul 25, 2021

Checklist:

Fixes #4772. Basically the problem is that the global parameters/artefacts are captured and stored in the template validation context normally, but the only the template scope, which does not contain the captured global params/artefacts, is checked in resolveAllVariables. resolveAllVariables checks the current scope, but nested templates have their own scope, and the global parameters/artefacts captured are not reflected in the upper level scope (only in the template validation context). Therefore, when a template is being validated by resolveAllVariables, it cannot find in the current scope the global parameters/artefacts resolved in templates in the next level.

-- level 1, template 1            -- scope 1, global context
      |
      -- level 2, template 2      -- scope 2, global context
-- level 1, template 3            -- scope 1, global context

Test

Executed the sample workflow mentioned in the issue successfully.

Signed-off-by: Huan-Cheng Chang <changhc84@gmail.com>
@codecov
Copy link

codecov bot commented Jul 25, 2021

Codecov Report

Merging #6422 (29d5a2f) into master (96b165e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6422      +/-   ##
==========================================
+ Coverage   48.74%   48.75%   +0.01%     
==========================================
  Files         253      253              
  Lines       18217    18218       +1     
==========================================
+ Hits         8880     8883       +3     
+ Misses       8367     8363       -4     
- Partials      970      972       +2     
Impacted Files Coverage Δ
workflow/validate/validate.go 76.19% <100.00%> (+0.86%) ⬆️
cmd/argo/commands/get.go 59.18% <0.00%> (-0.59%) ⬇️
workflow/controller/operator.go 72.40% <0.00%> (-0.15%) ⬇️

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 96b165e...29d5a2f. Read the comment docs.

…lows

Signed-off-by: Huan-Cheng Chang <changhc84@gmail.com>
@@ -548,7 +548,8 @@ func resolveAllVariables(scope map[string]interface{}, tmplStr string) error {
return nil
}
_, ok := scope[tag]
if !ok {
_, isGlobal := globalParams[tag]
if !ok && !isGlobal {
Copy link
Contributor

Choose a reason for hiding this comment

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

tests?

Copy link
Contributor Author

@changhc changhc Jul 26, 2021

Choose a reason for hiding this comment

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

Sorry. Added a test case to workflow/validate/validate_test.go and verified that it failed without the changes in this PR.
Also added more details about the root cause to the PR description.

Signed-off-by: Huan-Cheng Chang <changhc84@gmail.com>
Copy link
Member

@sarabala1979 sarabala1979 left a comment

Choose a reason for hiding this comment

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

LGTM

@alexec alexec merged commit dd3c112 into argoproj:master Jul 27, 2021
This was referenced Jul 27, 2021
mszostok added a commit to capactio/capact that referenced this pull request Oct 27, 2021
- Remove copied code from argo watch cmd. Now `capact act watch` command can be stopped via `ctrl+c` and it also prints the status icons.
- Bump to Argo Helm Chart
- Remove custom Argo images  as the bug was resolved, see: argoproj/argo-workflows#6422
- Bump k8s deps in `go.mod`
-  Change step name to lowercase as it's used for k8s pod name:
    ```
    Pod "e2e-test-1-8081-output-testUpload-3658329211" is invalid: metadata.name: Invalid value: "e2e-test-1-8081-output-testUpload-3658329211": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')
    ````
    Change was introduced in: argoproj/argo-workflows@8897fff
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.

Unable to resolve global artifacts created in nested workflows
3 participants