Fix issue with variables not substituted in `uri`-referenced manifests (#5451) #5711

rm3l · 2022-05-02T15:42:08Z

What type of PR is this:
/kind bug

What does this PR do / why we need it:

This fixes the issue reported in #5451, where running odo deploy (actually any commands calling libdevfile.GetK8sComponentAsUnstructured) using a Devfile containing uri-referenced manifests containing variables would not succeed, either with a YAML validation error message similar to the line below or with an error (e.g., unresolvable image name) in the manifest once deployed in the cluster:

error converting YAML to JSON: yaml: invalid map key: map[interface {}]interface {}{"appname":interface {}(nil)}

Which issue(s) this PR fixes:
Fixes #5451

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:
Refer to the reproduction steps depicted in the original issue for testing this: #5451

From the code perspective, as discussed during one of the Sprint Plannings, this PR is enriching the libdevfile package with a new GetK8sManifestWithVariablesSubstituted function that is responsible for returning the resource manifest from the Kubernetes/OpenShift Devfile component with all variables substituted, rather than having libdevfile.GetK8sComponentAsUnstructured (and few other places in the codebase) fetch the raw manifest from the URI and handle it as is.
Also, the implementation aims at leaving room for supporting OpenShift components in a near future (scope of #5704).

Since this seems a much more realistic scenario of how outer-loop manifests are referenced in Devfiles, this PR also switches to uri references in all test Devfiles. This is the purpose of df1f19e
Note that tests for odo deploy still test both inlined (as before) and uri manifests (scope of this issue). [Will be covered in a separate PR]

netlify · 2022-05-02T15:42:13Z

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name	Link
🔨 Latest commit	`614bc60`
🔍 Latest deploy log	https://app.netlify.com/sites/odo-docusaurus-preview/deploys/62738081a1a9880008a3189a

odo-robot · 2022-05-02T15:49:26Z

Unit Tests on commit 98d19f6 finished successfully.
View logs: TXT HTML

odo-robot · 2022-05-02T15:58:52Z

Windows Tests (OCP) on commit 98d19f6 finished successfully.
View logs: TXT HTML

odo-robot · 2022-05-02T16:01:11Z

Kubernetes Tests on commit 98d19f6 finished successfully.
View logs: TXT HTML

odo-robot · 2022-05-02T16:03:35Z

OpenShift Tests on commit 98d19f6 finished successfully.
View logs: TXT HTML

odo-robot · 2022-05-02T16:07:11Z

Validate Tests on commit 98d19f6 finished successfully.
View logs: TXT HTML

pkg/libdevfile/libdevfile.go

tests/integration/devfile/cmd_devfile_build_images_test.go

feloy

You have moved devfile from using Inlined to using URI as discussed during issue preparation. Are there some remaining devfile using Inlined? I would not like we make regressions on this now :(

pkg/libdevfile/errors.go

rm3l · 2022-05-03T15:27:02Z

You have moved devfile from using Inlined to using URI as discussed during issue preparation. Are there some remaining devfile using Inlined? I would not like we make regressions on this now :(

I identified the following places:

devfile-deploy-with-k8s-inlined.yaml (actually, it is just the old devfile-deploy.yaml file that I renamed). Renaming it was intentional to make sure 'Inlined' manifests could also be tested with odo deploy as before.
devfile-deploy-with-multiple-resources.yaml : this one can't use URI references at this time because of odo delete fails for kubernetes components #5637. I added TODO comments in this file.

I was also hesitant on changing this in this PR. That's why the commit is isolated if we want to revert or do this change in a separate issue/PR.

Ideally, I think we should come up with a way to easily reuse the same tests against both Inlined and URI-referenced manifests. This is what I tried to accomplish with this loop in cmd_devfile_deploy_test.go.

rm3l · 2022-05-04T14:58:55Z

You have moved devfile from using Inlined to using URI as discussed during issue preparation. Are there some remaining devfile using Inlined? I would not like we make regressions on this now :(

I identified the following places:

devfile-deploy-with-k8s-inlined.yaml (actually, it is just the old devfile-deploy.yaml file that I renamed). Renaming it was intentional to make sure 'Inlined' manifests could also be tested with odo deploy as before.

devfile-deploy-with-multiple-resources.yaml : this one can't use URI references at this time because of odo delete fails for kubernetes components #5637. I added TODO comments in this file.

I was also hesitant on changing this in this PR. That's why the commit is isolated if we want to revert or do this change in a separate issue/PR.

Ideally, I think we should come up with a way to easily reuse the same tests against both Inlined and URI-referenced manifests. This is what I tried to accomplish with this loop in cmd_devfile_deploy_test.go.

To be safe, I just reverted that commit from this PR, but saved it to a separate branch, and will try to include it in a dedicated PR later on.

feloy · 2022-05-05T07:40:40Z

/approve

openshift-ci · 2022-05-05T07:40:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [feloy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

The devfile used is the same as devfile-deploy.yaml, except that the the Kubernetes component is referenced via the URI. This is why the same integration test logic is being reused here.

…resource manifest and substitute variables This works with both Inlined or Uri resources. Notes: - Ideally, it would make more sense to rely on the same logic used in Devfile to substitute variables (defined in the 'variables' package), but this function is not exported; and the exported ones substitute variables only in the URI name, not in the content itself (it is not fetched), which is actually the issue we are trying to solve here.

This seems to be a much more realistic case when referencing external Kubernetes manifests. Notes: Some tests, like for `odo deploy`, still test 'Inlined' manifests.

…riablesResolved' As suggested in review, this makes more sense now that we are passing the entire devfile object

…h 'GetK8sManifestWithVariablesSubstituted' Previous name was a bit long ^^

…ameNameError#Error', as suggested in review

This reverts commit df1f19e. This will be done in a separate PR, with ideally changes that should allow to use the same set of tests for testing both Inlined and URI-referenced manifests.

rm3l · 2022-05-05T07:46:14Z

Just rebased onto main to benefit from #5712, which fixed the tests.

sonarqubecloud · 2022-05-05T07:46:34Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

feloy · 2022-05-05T09:47:03Z

/lgtm

rm3l · 2022-05-05T11:18:32Z

/override ci/prow/v4.10-integration-e2e

Looks like a problem not related to this PR, but related to the deprovisioning job that tears down the cluster on Prow:

could not run steps: step integration-e2e failed: "integration-e2e" post steps failed: "integration-e2e" pod "integration-e2e-ipi-deprovision-deprovision" failed: the pod ci-op-kzni8cn5/integration-e2e-ipi-deprovision-deprovision was deleted without completing after 4m50s (failed containers: )

All tests passed on IBM Cloud by the way.

openshift-ci · 2022-05-05T11:18:56Z

@rm3l: Overrode contexts on behalf of rm3l: ci/prow/v4.10-integration-e2e

In response to this:

/override ci/prow/v4.10-integration-e2e

Looks like a problem not related to this PR, but related to the deprovisioning job that tears down the cluster on Prow:

could not run steps: step integration-e2e failed: "integration-e2e" post steps failed: "integration-e2e" pod "integration-e2e-ipi-deprovision-deprovision" failed: the pod ci-op-kzni8cn5/integration-e2e-ipi-deprovision-deprovision was deleted without completing after 4m50s (failed containers: )

All tests passed on IBM Cloud by the way.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

redhat-developer#5451) (redhat-developer#5711) * Add integration test highlighting the issue The devfile used is the same as devfile-deploy.yaml, except that the the Kubernetes component is referenced via the URI. This is why the same integration test logic is being reused here. * Add higher-level function in 'libdevfile' allowing to load component resource manifest and substitute variables This works with both Inlined or Uri resources. Notes: - Ideally, it would make more sense to rely on the same logic used in Devfile to substitute variables (defined in the 'variables' package), but this function is not exported; and the exported ones substitute variables only in the URI name, not in the content itself (it is not fetched), which is actually the issue we are trying to solve here. * Switch to using 'uri' Kubernetes components in test Devfiles This seems to be a much more realistic case when referencing external Kubernetes manifests. Notes: Some tests, like for `odo deploy`, still test 'Inlined' manifests. * Pass the component name to 'GetComponentResourceManifestContentWithVariablesResolved' As suggested in review, this makes more sense now that we are passing the entire devfile object * Rename 'GetComponentResourceManifestContentWithVariablesResolved' with 'GetK8sManifestWithVariablesSubstituted' Previous name was a bit long ^^ * Remove extra parentheses in error string returned by 'ComponentsWithSameNameError#Error', as suggested in review * Revert "Switch to using 'uri' Kubernetes components in test Devfiles" This reverts commit df1f19e. This will be done in a separate PR, with ideally changes that should allow to use the same set of tests for testing both Inlined and URI-referenced manifests.

rm3l requested review from dharmit, feloy and valaparthvi May 2, 2022 15:42

openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label May 2, 2022

feloy suggested changes May 3, 2022

View reviewed changes

pkg/libdevfile/libdevfile.go Outdated Show resolved Hide resolved

tests/integration/devfile/cmd_devfile_build_images_test.go Outdated Show resolved Hide resolved

openshift-ci bot assigned feloy May 3, 2022

feloy reviewed May 3, 2022

View reviewed changes

pkg/libdevfile/errors.go Outdated Show resolved Hide resolved

rm3l requested a review from feloy May 4, 2022 06:57

feloy approved these changes May 4, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label May 4, 2022

rm3l changed the title ~~Fix issue with variable substitution not occurring in uri-referenced manifests (#5451)~~ Fix issue with variables not substituted in uri-referenced manifests (#5451) May 4, 2022

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label May 4, 2022

feloy approved these changes May 4, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label May 4, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label May 5, 2022

rm3l added 5 commits May 5, 2022 09:44

Add integration test highlighting the issue

273e38c

The devfile used is the same as devfile-deploy.yaml, except that the the Kubernetes component is referenced via the URI. This is why the same integration test logic is being reused here.

Switch to using 'uri' Kubernetes components in test Devfiles

0c03bdf

This seems to be a much more realistic case when referencing external Kubernetes manifests. Notes: Some tests, like for `odo deploy`, still test 'Inlined' manifests.

Pass the component name to 'GetComponentResourceManifestContentWithVa…

514c90f

…riablesResolved' As suggested in review, this makes more sense now that we are passing the entire devfile object

Rename 'GetComponentResourceManifestContentWithVariablesResolved' wit…

46d9411

…h 'GetK8sManifestWithVariablesSubstituted' Previous name was a bit long ^^

rm3l added 2 commits May 5, 2022 09:44

Remove extra parentheses in error string returned by 'ComponentsWithS…

951ec8c

…ameNameError#Error', as suggested in review

Revert "Switch to using 'uri' Kubernetes components in test Devfiles"

614bc60

This reverts commit df1f19e. This will be done in a separate PR, with ideally changes that should allow to use the same set of tests for testing both Inlined and URI-referenced manifests.

rm3l force-pushed the 5451-odo-deploy-devfile-variable-substitution-does-not-occur-in-uri-referenced-deployment-manifest-yaml-file branch from 9d78b5c to 614bc60 Compare May 5, 2022 07:45

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label May 5, 2022

rm3l requested a review from feloy May 5, 2022 08:49

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label May 5, 2022

openshift-merge-robot merged commit 7c7e607 into redhat-developer:main May 5, 2022

rm3l mentioned this pull request Sep 1, 2022

devfile variables inside kubernetes component are not replaced #5240

Closed

rm3l deleted the 5451-odo-deploy-devfile-variable-substitution-does-not-occur-in-uri-referenced-deployment-manifest-yaml-file branch December 1, 2022 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with variables not substituted in `uri`-referenced manifests (#5451) #5711

Fix issue with variables not substituted in `uri`-referenced manifests (#5451) #5711

rm3l commented May 2, 2022 •

edited

Loading

netlify bot commented May 2, 2022 •

edited

Loading

odo-robot bot commented May 2, 2022 •

edited

Loading

odo-robot bot commented May 2, 2022 •

edited

Loading

odo-robot bot commented May 2, 2022 •

edited

Loading

odo-robot bot commented May 2, 2022 •

edited

Loading

odo-robot bot commented May 2, 2022 •

edited

Loading

feloy left a comment

rm3l commented May 3, 2022

rm3l commented May 4, 2022

feloy commented May 5, 2022

openshift-ci bot commented May 5, 2022

rm3l commented May 5, 2022

sonarqubecloud bot commented May 5, 2022

feloy commented May 5, 2022

rm3l commented May 5, 2022

openshift-ci bot commented May 5, 2022

Fix issue with variables not substituted in uri-referenced manifests (#5451) #5711

Fix issue with variables not substituted in uri-referenced manifests (#5451) #5711

Conversation

rm3l commented May 2, 2022 • edited Loading

netlify bot commented May 2, 2022 • edited Loading

✅ Deploy Preview for odo-docusaurus-preview canceled.

odo-robot bot commented May 2, 2022 • edited Loading

odo-robot bot commented May 2, 2022 • edited Loading

odo-robot bot commented May 2, 2022 • edited Loading

odo-robot bot commented May 2, 2022 • edited Loading

odo-robot bot commented May 2, 2022 • edited Loading

feloy left a comment

Choose a reason for hiding this comment

rm3l commented May 3, 2022

rm3l commented May 4, 2022

feloy commented May 5, 2022

openshift-ci bot commented May 5, 2022

rm3l commented May 5, 2022

sonarqubecloud bot commented May 5, 2022

feloy commented May 5, 2022

rm3l commented May 5, 2022

openshift-ci bot commented May 5, 2022

Fix issue with variables not substituted in `uri`-referenced manifests (#5451) #5711

Fix issue with variables not substituted in `uri`-referenced manifests (#5451) #5711

rm3l commented May 2, 2022 •

edited

Loading

netlify bot commented May 2, 2022 •

edited

Loading

odo-robot bot commented May 2, 2022 •

edited

Loading

odo-robot bot commented May 2, 2022 •

edited

Loading

odo-robot bot commented May 2, 2022 •

edited

Loading

odo-robot bot commented May 2, 2022 •

edited

Loading

odo-robot bot commented May 2, 2022 •

edited

Loading