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.

Sign up for GitHub

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

Jump to bottom

Substituting variables into the devfile from the CLI #5749

Merged
merged 11 commits into from
Jun 13, 2022
Merged

Substituting variables into the devfile from the CLI #5749

merged 11 commits into from
Jun 13, 2022

Conversation

feloy
Copy link
Contributor

@feloy feloy commented May 18, 2022

What type of PR is this:

/kind feature

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #5489

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

@netlify
Copy link

netlify bot commented May 18, 2022 •
edited
Loading

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name Link
🔨 Latest commit 7201405
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/62a6efa36d965c000931e255

@feloy feloy marked this pull request as draft May 18, 2022 09:42
@openshift-ci openshift-ci bot added kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. labels May 18, 2022
@odo-robot
Copy link

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

Unit Tests on commit 2a3ed12 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

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

Windows Tests (OCP) on commit 2a3ed12 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

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

Kubernetes Tests on commit 2a3ed12 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

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

OpenShift Tests on commit 2a3ed12 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

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

Validate Tests on commit 2a3ed12 finished successfully.
View logs: TXT HTML

@feloy feloy added status/blocked Denotes an issue or PR that is blocked on something (e.g., issue/PR in different repo) and removed status/blocked Denotes an issue or PR that is blocked on something (e.g., issue/PR in different repo) labels Jun 2, 2022
@feloy feloy force-pushed the feature-5489/external-variables branch from 406b957 to 631e0c8 Compare June 2, 2022 13:47
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jun 2, 2022
@feloy feloy force-pushed the feature-5489/external-variables branch from ad6c4d8 to 18031d6 Compare June 2, 2022 14:41
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jun 2, 2022
@feloy feloy removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 2, 2022
@feloy feloy marked this pull request as ready for review June 2, 2022 14:43
@openshift-ci openshift-ci bot requested review from anandrkskd and dharmit June 2, 2022 14:43
@feloy feloy requested review from rm3l and dharmit June 2, 2022 14:43
@feloy feloy requested review from valaparthvi and removed request for valaparthvi, anandrkskd and rnapoles-rh June 2, 2022 14:43
rm3l
rm3l requested changes Jun 3, 2022
View reviewed changes
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

Added a few comments after a quick review. Yet to review the other files.

pkg/devfile/devfile.go Outdated Show resolved Hide resolved
docs/website/versioned_docs/version-3.0.0/command-reference/dev.md Outdated Show resolved Hide resolved
@feloy feloy requested a review from rm3l June 7, 2022 05:59
valaparthvi
valaparthvi reviewed Jun 9, 2022
View reviewed changes
Copy link
Contributor

@valaparthvi valaparthvi left a comment

Choose a reason for hiding this comment

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

Code LGTM apart from a few nitpicks.

pkg/vars/vars.go Outdated Show resolved Hide resolved
pkg/vars/vars.go Outdated Show resolved Hide resolved
pkg/vars/vars.go Outdated Show resolved Hide resolved
docs/website/versioned_docs/version-3.0.0/command-reference/dev.md
```shell
$ odo dev --var USER=john --var-file config.vars
```

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding a note stating that passing a KEY=VALUE for a key that does not exist in the devfile.yaml will be ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just made the test, and finally, it is taken into account. But it depends on the implementation of the devfile library, I would prefer not to document it

@valaparthvi
Copy link
Contributor

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jun 9, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: valaparthvi

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 9, 2022
@feloy @valaparthvi
Apply suggestions from code review
73947ff 
Co-authored-by: Parthvi Vala <pvala@redhat.com>
rm3l
rm3l requested changes Jun 13, 2022
View reviewed changes
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

Added one last comment on the code..

pkg/vars/vars.go Outdated Show resolved Hide resolved
@feloy feloy requested a review from rm3l June 13, 2022 08:14
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 13, 2022
@feloy feloy closed this Jun 13, 2022
@feloy feloy reopened this Jun 13, 2022
@sonarcloud
Copy link

sonarcloud bot commented Jun 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
1.3% 1.3% Duplication

@feloy
Copy link
Contributor Author

feloy commented Jun 13, 2022

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

IBM tests pass

@openshift-ci
Copy link

openshift-ci bot commented Jun 13, 2022

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

In response to this:

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

IBM tests pass

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.

@openshift-merge-robot openshift-merge-robot merged commit 8cdc934 into redhat-developer:main Jun 13, 2022
cdrage pushed a commit to cdrage/odo that referenced this pull request Aug 31, 2022
@feloy @valaparthvi @cdrage
Substituting variables into the devfile from the CLI (redhat-develope…
45bbbe9 
…r#5749)

* Define var and and-file flags

* vars package

* Vars for deploy

* Add integration tests for dev

* Update dev mock

* New devfile library version

* Add doc

* Fix validate

* Review

* Apply suggestions from code review

Co-authored-by: Parthvi Vala <pvala@redhat.com>

* review

Co-authored-by: Parthvi Vala <pvala@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valaparthvi valaparthvi valaparthvi left review comments

@rm3l rm3l rm3l approved these changes

@dharmit dharmit Awaiting requested review from dharmit

Assignees

@rm3l rm3l

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Substituting variables into the devfile from the CLI
4 participants
@feloy @valaparthvi @rm3l @openshift-merge-robot