Skip to content

Send correct params for starting a pipeline - #3286

Merged
openshift-merge-robot merged 2 commits into
openshift:masterfrom
rohitkrai03:bug-odc-2204
Nov 12, 2019
Merged

Send correct params for starting a pipeline#3286
openshift-merge-robot merged 2 commits into
openshift:masterfrom
rohitkrai03:bug-odc-2204

Conversation

@rohitkrai03

@rohitkrai03 rohitkrai03 commented Nov 7, 2019

Copy link
Copy Markdown
Contributor

Related Bug - https://jira.coreos.com/browse/ODC-2204

This PR -

  • Fixes the params being sent in the PipelineRun for starting a pipeline.

@rohitkrai03

rohitkrai03 commented Nov 7, 2019

Copy link
Copy Markdown
Contributor Author

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 7, 2019
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. component/dev-console Related to dev-console approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 7, 2019
andrewballantyne
andrewballantyne suggested changes Nov 7, 2019
View reviewed changes

@andrewballantyne andrewballantyne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a couple things to tidy up (*cough* tests *cough*)

Comment thread frontend/packages/dev-console/src/utils/pipeline-utils.ts Outdated

@andrewballantyne andrewballantyne Nov 7, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

*cough* tests *cough*

@rohitkrai03 rohitkrai03 Nov 11, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added unit test.

Comment thread frontend/packages/dev-console/src/utils/pipeline-actions.tsx Outdated

@andrewballantyne andrewballantyne Nov 7, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
...(params && { params }),
params,

You're always giving this an array. (I'd move this also back to where it was for cleaner diffs)

getPipelineRunParams always returns an array (.map) and empty arrays are truthy.

@rohitkrai03 rohitkrai03 Nov 11, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@rohitkrai03

rohitkrai03 commented Nov 11, 2019

Copy link
Copy Markdown
Contributor Author

/retest

andrewballantyne
andrewballantyne suggested changes Nov 12, 2019
View reviewed changes

@andrewballantyne andrewballantyne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but I think we should write an explicit test scenario to prevent the root cause here. We cannot send default up.

Comment thread frontend/packages/dev-console/src/utils/__tests__/pipeline-utils.spec.ts Outdated

@andrewballantyne andrewballantyne Nov 12, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe also:

Suggested change
expect(params[0].value).toBe('default-app-name');
expect(params[0].value).toBe('default-app-name');
expect(params[0].default).toBeNotDefined();

We are really looking for this to never exist. Another test is also (and probably more-so) accepted.

@rohitkrai03 rohitkrai03 Nov 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Although we do not need to explicitly test for default not being there in params since I've added proper types for both PipelineParams and PipelineRunParams, I've added an extra test in pipelines-actions.spec.ts to explicitly test it.

@andrewballantyne andrewballantyne Nov 12, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I prefer both tbh. Makes the change harder to accidentally do. But you're right, the type would prevent this issue.

@openshift-ci-robot openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2019
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 12, 2019
@rohitkrai03

rohitkrai03 commented Nov 12, 2019

Copy link
Copy Markdown
Contributor Author

@spadgett It seems the bot is not working correctly for any of the PRs. Now even for this PR its ignoring OWNERS file of dev-console which was not the case before I pushed new changes.

@rohitkrai03

rohitkrai03 commented Nov 12, 2019

Copy link
Copy Markdown
Contributor Author

/test e2e-gcp-console

andrewballantyne
andrewballantyne approved these changes Nov 12, 2019
View reviewed changes

@andrewballantyne andrewballantyne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2019
@openshift-ci-robot

openshift-ci-robot commented Nov 12, 2019

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, rohitkrai03

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

The pull request process is described here

Details 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. label Nov 12, 2019
@andrewballantyne

andrewballantyne commented Nov 12, 2019

Copy link
Copy Markdown
Contributor

@spadgett It seems the bot is not working correctly for any of the PRs. Now even for this PR its ignoring OWNERS file of dev-console which was not the case before I pushed new changes.

Bot hiccuped? It appears to be back.

@rohitkrai03

rohitkrai03 commented Nov 12, 2019

Copy link
Copy Markdown
Contributor Author

Yeah, seems to be back now.

@rohitkrai03

rohitkrai03 commented Nov 12, 2019

Copy link
Copy Markdown
Contributor Author

/test e2e-gcp-console

1 similar comment
@rohitkrai03

rohitkrai03 commented Nov 12, 2019

Copy link
Copy Markdown
Contributor Author

/test e2e-gcp-console

@openshift-merge-robot
openshift-merge-robot merged commit 47d9065 into openshift:master Nov 12, 2019
@spadgett spadgett added this to the v4.3 milestone Nov 19, 2019
@rohitkrai03
rohitkrai03 deleted the bug-odc-2204 branch March 30, 2020 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@divyanshiGupta divyanshiGupta Awaiting requested review from divyanshiGupta
@vikram-raj vikram-raj Awaiting requested review from vikram-raj
1 more reviewer
@andrewballantyne andrewballantyne andrewballantyne approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@andrewballantyne andrewballantyne

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

v4.3

Development

Successfully merging this pull request may close these issues.

5 participants

@rohitkrai03 @openshift-ci-robot @andrewballantyne @spadgett @openshift-merge-robot