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

Verify local config after component create #2457

Merged
merged 10 commits into from
Jan 7, 2020
Merged

Verify local config after component create #2457

merged 10 commits into from
Jan 7, 2020

Conversation

prietyc123
Copy link
Contributor

/kind test

What does does this PR do / why we need it:
Verifies local component config on the host.
Which issue(s) this PR fixes:

Fixes #2439

How to test changes / Special notes to the reviewer:
$ make test-cmd-cmp && make test-cmd-cmp-sub

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/test labels Dec 13, 2019
@prietyc123
Copy link
Contributor Author

/test v4.1-integration-e2e-benchmark

@prietyc123 prietyc123 force-pushed the VerifyLocalConfigAfterCmpCreate branch 3 times, most recently from 5556498 to 403dbf0 Compare December 16, 2019 16:51
@prietyc123
Copy link
Contributor Author 

https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_odo/2457/pull-ci-openshift-odo-master-v4.1-integration-e2e-benchmark/212#1:build-log.txt%3A290

/retest

@prietyc123 prietyc123 force-pushed the VerifyLocalConfigAfterCmpCreate branch from 403dbf0 to bd82546 Compare December 18, 2019 15:03
@prietyc123 prietyc123 changed the title [WIP] Verify local config after component create Verify local config after component create Dec 18, 2019
@openshift-ci-robot openshift-ci-robot 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 Dec 18, 2019
@prietyc123 prietyc123 force-pushed the VerifyLocalConfigAfterCmpCreate branch 2 times, most recently from a137578 to 263e649 Compare December 18, 2019 16:50
amitkrout
amitkrout suggested changes Dec 19, 2019
View reviewed changes
tests/integration/component.go Outdated
@@ -112,6 +119,8 @@ func componentTests(args ...string) {
It("should list out component in json format along with path flag", func() {
var contextPath string
helper.CmdShouldPass("odo", append(args, "create", "nodejs", "nodejs", "--project", project)...)
cmpSetting := helper.VerifyLocalConfig(context + "/.odo/config.yaml")
Copy link
Contributor

@amitkrout amitkrout Dec 19, 2019 •
edited

Choose a reason for hiding this comment

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

Use filepath.Join() in all occurrence. For example

filepath.Join(context, ".odo", "config.yaml")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use filepath.Join() in all occurrence. For example

filepath.Join(context, ".odo", "config.yaml")

Done 👍

@prietyc123 prietyc123 force-pushed the VerifyLocalConfigAfterCmpCreate branch 2 times, most recently from 1a4f371 to 20dc922 Compare December 20, 2019 09:08
@prietyc123 prietyc123 changed the title Verify local config after component create [WIP]Verify local config after component create Dec 20, 2019
@openshift-ci-robot openshift-ci-robot added 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 Dec 20, 2019
@prietyc123 prietyc123 force-pushed the VerifyLocalConfigAfterCmpCreate branch from 20dc922 to 341362f Compare December 23, 2019 14:34
@prietyc123 prietyc123 changed the title [WIP]Verify local config after component create Verify local config after component create Dec 23, 2019
@openshift-ci-robot openshift-ci-robot 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 Dec 23, 2019
@prietyc123 prietyc123 mentioned this pull request Dec 24, 2019
Closed
@prietyc123 prietyc123 force-pushed the VerifyLocalConfigAfterCmpCreate branch from 341362f to 0386791 Compare December 30, 2019 12:42
mik-dass
mik-dass reviewed Jan 2, 2020
View reviewed changes
tests/helper/helper_configValidate.go
for i := 0; i < len(args); i++ {
keyValue := strings.Split(args[i], ",")

if Search(cmpField, keyValue[0]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please add some comments for the next steps from here

@prietyc123 prietyc123 force-pushed the VerifyLocalConfigAfterCmpCreate branch from 4a2379c to 649f1fd Compare January 2, 2020 10:28
@prietyc123
Copy link
Contributor Author

/retest

@girishramnani
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jan 3, 2020
@amitkrout
Copy link
Contributor

Changes looks good to me. @prietyc123 please rebase it

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels Jan 6, 2020
@prietyc123 prietyc123 force-pushed the VerifyLocalConfigAfterCmpCreate branch from 7260eb4 to 113561c Compare January 7, 2020 09:40
@amitkrout
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jan 7, 2020
@openshift-merge-robot openshift-merge-robot merged commit 7647d90 into redhat-developer:master Jan 7, 2020
@rm3l rm3l added the estimated-size/L (20-40) Rough sizing for Epics. About 2 sprints of work for a person. label Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@girishramnani girishramnani girishramnani left review comments

@mik-dass mik-dass mik-dass left review comments

@amitkrout amitkrout amitkrout requested changes

@mohammedzee1000 mohammedzee1000 Awaiting requested review from mohammedzee1000

Assignees

@amitkrout amitkrout

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. estimated-size/L (20-40) Rough sizing for Epics. About 2 sprints of work for a person. 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.

Verify the local config after component creation
7 participants
@prietyc123 @girishramnani @openshift-ci-robot @amitkrout @mik-dass @rm3l @openshift-merge-robot