step-registry: setup the commands to allow running on bastion #9640
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya 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 |
openshift-ci-robot
added
the
approved
| @@ -4,6 +4,10 @@ set -o nounset | |||
| set -o errexit | |||
| set -o pipefail | |||
|
|
|||
| export HOME=/tmp | |||
| export WORKSPACE=${WORKSPACE:-/tmp} | |||
There was a problem hiding this comment.
Do you want this to be something generically available to all steps?
There was a problem hiding this comment.
I think that would be useful imo. making sure we have the workspace and path setup like this.
There was a problem hiding this comment.
Can we use TMPDIR, which is in POSIX?
There was a problem hiding this comment.
We can also lean on openshift/ci-tools#854 and define variables for these (example in #9676) to keep them out of the script.
There was a problem hiding this comment.
Can we use TMPDIR, which is in POSIX?
i like WORKSPACE name more beacuse it is more clearer for it's responsibility. and works better when using it in jumped formed.
There was a problem hiding this comment.
We can also lean on openshift/ci-tools#854 and define variables for these (example in #9676) to keep them out of the script.
The step env variables are user configurable inputs to the steps, so using it for something that is more about core function, i.e. where do i create my files seems like a mis use. I would never want someone to change the location of the workspace in a workflow because it is proabably going to break the step as we don't create or check permissions for that dir it is given to us.
There was a problem hiding this comment.
i like WORKSPACE name more beacuse it is more clearer for it's responsibility. and works better when using it in jumped formed.
Is #9553 actually setting WORKSPACE anywhere?
The step env variables are user configurable inputs to the steps...
And they're also a way to set convenient defaults, for things where the script has no opinion.
I would never want someone to change the location of the workspace in a workflow because it is proabably going to break the step as we don't create or check permissions for that dir it is given to us.
Then just hard-code the value, and skip the variable? If this is something where you have some consumers that want a knob (e.g. some #9553 logic I'm missing about running these jobs with a different WORKSPACE on a bastion), then add an environment knob. And if callers point it at a non-existent directory or whatever, and the job breaks as a result, that's the caller's problem. I don't think we need defensive guards for things like that, and I don't think the lack of defensive guards is a reason to avoid a declared env variable in these internal tools.
There was a problem hiding this comment.
I think exposing the env from steps that are not actually user-configurable or changing them just breaks is bad API. So I do not think workspace is a good idea.
Also #9553 only uses the bash script itself and not the step, it copies the bash script to the bastion and then runs it using a unique workspace. I do not need these to available as an API on the steps level just to these scripts for reusing.
|
/retest |
abhinavdahiya
force-pushed
the
workspace-steps
branch
from
d5d4e66 to
21a242e
Compare
|
/retitle step-registry: setup the commands to allow running on bastion Fixing "bastian" -> "bastion". |
openshift-ci-robot
changed the title
| export PATH=$PATH:/tmp/shared | ||
| export HOME=/tmp | ||
| export WORKSPACE=${WORKSPACE:-/tmp} | ||
| export PATH=${PATH}:${WORKSPACE} |
There was a problem hiding this comment.
While we're touching this and adding new lines, we should quote (e.g. export PATH="${PATH}:${WORKSPACE}"). And maybe also grow ShellCheck coverage for ci-operator/step-registry?
There was a problem hiding this comment.
And maybe also grow ShellCheck coverage for ci-operator/step-registry?
not sure what you mean by that... can provide a little more detail more than happy to extend coverage.
While we're touching this and adding new lines, we should quote (e.g. export PATH="${PATH}:${WORKSPACE}").
Sure will do.
There was a problem hiding this comment.
And maybe also grow ShellCheck coverage for ci-operator/step-registry?
not sure what you mean by that...
The installer has ShellCheck CI to keep us from making quoting mistakes in openshift/installer. We should have similar CI for this repo (but I can file a PR for that separately; it's orthogonal to this PR).
There was a problem hiding this comment.
Huh, seems like we already do. This job ran with d5d4e66776's unquoted PATH value and did not fail. Hmm... are we running a ShellCheck image without mounting in our openshift/release checkout?
To enable internal clusters current plan is to run the steps from the bastian instances. Since there can be multiple of these running to isolate the binaries and files this change makes 2 changes, - WORSPACE, this defaults to /tmp All the files created as part of the steps, that are not in shared or artifact dir must be created in the WORKSPACE dir. This ensures that on bastians we can provide each test separate dir for the files. - PATH WORKSPACE is added to the PATH, this allows the wrapper to copy all the required binaries for that test to the WORKSPACE and the step can continue to use these transparently.
abhinavdahiya
force-pushed
the
workspace-steps
branch
from
21a242e to
4ea92d9
Compare
|
Given the redundant setting of environment variables across scripts in this, and in #9553 , I think it would make more sense to set them globally in another file and source it. Any overrides can be done in individual scripts. |
We can't yet do that, reuse files across steps. |
|
@abhinavdahiya: PR needs rebase. 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-ci-robot
added
the
needs-rebase
|
@abhinavdahiya: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@abhinavdahiya: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@abhinavdahiya: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci-robot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. In response to this:
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. |
To enable internal clusters current plan is to run the steps from the bastian instances. Since there can be multiple of these running to isolate the binaries and files
this change makes 2 changes,
All the files created as part of the steps, that are not in shared or artifact dir must be created in the WORKSPACE dir. This ensures that on bastians we can provide each test separate dir for the files.
WORKSPACE is added to the PATH, this allows the wrapper to copy all the required binaries for that test to the WORKSPACE and the step can continue to use these transparently.