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

feat: Allow composite commands to be overridden #4043

Merged

Conversation

johnmcollier
Copy link
Member

What type of PR is this?
/kind feature
/area devfile

What does does this PR do / why we need it:
Adds support for overriding composite commands in parent devfiles. Also fixes a bug where parent commands could not be overridden if the parent command contained upper case letters.

Which issue(s) this PR fixes:

Fixes #3759

PR acceptance criteria:

How to test changes / Special notes to the reviewer:
Sample devfile:

schemaVersion: 2.0.0
metadata:
  name: nodejs
  version: 1.0.0
parent:
  uri: https://raw.githubusercontent.com/openshift/odo/master/tests/examples/source/devfiles/nodejs/devfileCompositeCommands.yaml
  commands:
  - id: buildAndMkdir
    composite:
         label: Build and Mkdir
         commands:
           - createfile
           - install
         parallel: false
         group: 
            kind: build
            isDefault: true
commands:
  - id: createfile
    exec:
      component: runtime
      commandLine: touch /projects/testfile
      workingDir: ${PROJECTS_ROOT}
      group:
        kind: build
        isDefault: false

Output should have:

Executing devfile commands for component nodejs
 ✓  Executing createfile command "touch /projects/testfile" [300ms]
 ✓  Executing install command "npm install" [2s]
 ✓  Executing run command "npm start" [1s]

Adds support for overriding composite commands in parent devfiles. Also fixes a bug where parent commands could not be overridden if the parent command contained upper case letters.

Signed-off-by: John Collier <[email protected]>
@openshift-ci-robot openshift-ci-robot added kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation area/devfile-spec Issues or PRs related to the Devfile specification and how odo handles and interprets it. labels Sep 25, 2020
pkg/devfile/parser/types.go Outdated Show resolved Hide resolved
Signed-off-by: John Collier <[email protected]>
Signed-off-by: John Collier <[email protected]>
@johnmcollier
Copy link
Member Author

/retest

Signed-off-by: John Collier <[email protected]>
@johnmcollier
Copy link
Member Author

@mik-dass Thanks for reviewing, fixed the typos! The "o" key on my laptop has been acting up lately 😅

@codecov
Copy link

codecov bot commented Sep 29, 2020

Codecov Report

Merging #4043 into master will increase coverage by 0.02%.
The diff coverage is 57.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4043      +/-   ##
==========================================
+ Coverage   43.28%   43.31%   +0.02%     
==========================================
  Files         147      147              
  Lines       12435    12452      +17     
==========================================
+ Hits         5383     5394      +11     
- Misses       6483     6486       +3     
- Partials      569      572       +3     
Impacted Files Coverage Δ
pkg/devfile/parser/data/2.0.0/components.go 40.84% <0.00%> (ø)
pkg/devfile/parser/types.go 64.70% <60.00%> (ø)

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 19098a2...ee229e3. Read the comment docs.

Copy link
Contributor

@mik-dass mik-dass left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 30, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mik-dass

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 Sep 30, 2020
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@mik-dass mik-dass removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 30, 2020
Copy link
Member

@cdrage cdrage 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!

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Oct 6, 2020
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Oct 7, 2020
@kadel
Copy link
Member

kadel commented Oct 7, 2020

re-adding
/lgtm
after rebase

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Oct 7, 2020
@openshift-merge-robot openshift-merge-robot merged commit 49853dc into redhat-developer:master Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. area/devfile-spec Issues or PRs related to the Devfile specification and how odo handles and interprets it. 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
Development

Successfully merging this pull request may close these issues.

Add support for overriding composite commands
9 participants