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

Disable preStart event using validations #4405

Merged

Conversation

kadel
Copy link
Member

@kadel kadel commented Feb 3, 2021

What type of PR is this?

Uncomment only one /kind line, and delete the rest.
For example, > /kind bug would simply become: /kind bug

/kind bug

What does does this PR do / why we need it:

preStart even is still not clearly defined in Devfile and it is clear how it should be defined. This will make sure that no one is using PreStart event until we have a proper implementation.

Which issue(s) this PR fixes:

Fixes #4187

PR acceptance criteria:

How to test changes / Special notes to the reviewer:
Create a devfile with the preStart event command

events:
  preStart:
     - some-command

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 3, 2021
@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 Feb 3, 2021
Copy link
Member

@dharmit dharmit left a comment

Choose a reason for hiding this comment

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

Something's off here. When I do odo push, I see:

$ odo push        
 ✗  devfile events validation error: 
preStart type events are invalid: 
some-command does not map to a valid devfile command

I expected to see something along the lines of:

$ odo push
✗  devfile events validation error: 
"preStart" type events are not supported in odo

pkg/devfile/validate/errors.go Outdated Show resolved Hide resolved
@dharmit
Copy link
Member

dharmit commented Feb 4, 2021

Also, are the integration tests written with the assumption that PreStart events should work fine? 🤔

 • Failure [2.025 seconds]
odo devfile push command tests
/go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_push_test.go:20
  Verify devfile push works
  /go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_push_test.go:64
    should execute PreStart commands if present during pod startup [It]
    /go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_push_test.go:365
    No future change is possible.  Bailing out early after 0.071s.
      
    Running odo with args [odo push --project tmpjpmfnmc]
    Expected
        <int>: 1
    to match exit code:
        <int>: 0

@kadel kadel changed the title Disable preStart event using validations [WIP] Disable preStart event using validations Feb 4, 2021
@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 Feb 4, 2021
@kadel
Copy link
Member Author

kadel commented Feb 4, 2021

Something's off here. When I do odo push, I see:

$ odo push        
 ✗  devfile events validation error: 
preStart type events are invalid: 
some-command does not map to a valid devfile command

This is actually correct. This is a validation that comes from devfile/library.
It means that the command that you are using in preStart even doesn't exist in commands definition. You can't reference non-existing command in events. You basically created an invalid devfile, so it fails before it gets into the odo specific validations.

@dharmit
Copy link
Member

dharmit commented Feb 4, 2021

It means that the command that you are using in preStart even doesn't exist in commands definition. You can't reference non-existing command in events.

Second time in less than 24 hours that I misunderstood or didn't know what command means in devfile lingo. I did the same on #4381 (comment). This indicates two things to me:

  1. I obviously don't understand the devfile API as well as people who designed it (you) or integrated it with odo (@mik-dass).
  2. We need to make this obvious/clear in devfile documentation so that people don't fumble like I did. I'll leave this for you to take up with devfile team if it makes sense.

@dharmit
Copy link
Member

dharmit commented Feb 5, 2021

Some rebase mess up here #4405 (commits)?

preStart even is still not clearly defined in Devfile and it is clear how it should be defined. This will make sure that noone is using PreStart event until we have a proper implementation.
@kadel kadel changed the title [WIP] Disable preStart event using validations Disable preStart event using validations Feb 5, 2021
@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 Feb 5, 2021
@kadel
Copy link
Member Author

kadel commented Feb 5, 2021

fixed

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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

Copy link
Member

@dharmit dharmit left a comment

Choose a reason for hiding this comment

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

Just a question about comment vs. deleting. But it's not a show stopper.

/lgtm

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

/retest

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

@kadel
Copy link
Member Author

kadel commented Feb 5, 2021

[odo] : error while streaming command: error sending request: Post https://api.ci-op-tl8sj0lj-de358.origin-ci-int-aws.dev.rhcloud.com:6443/api/v1/namespaces/avacaawobu/pods/xwslrp-7c86484c96-wgkxk/exec?command=%2Fbin%2Fsh&command=-c&command=cd+%24%7BPROJECTS_ROOT%7D+%26%26+npm+install&container=runtime&stderr=true&stdout=true: dial tcp: lookup api.ci-op-tl8sj0lj-de358.origin-ci-int-aws.dev.rhcloud.com on 172.30.0.10:53: read udp 10.129.43.239:44972->172.30.0.10:53: i/o timeout

@openshift-merge-robot openshift-merge-robot merged commit 36f473e into redhat-developer:master Feb 5, 2021
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. kind/bug Categorizes issue or PR as related to a bug. 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.

Odo should use apply command for preStart(and postStop when its implemented)
5 participants