fix: (different approach) makefile temp logic #1391
Conversation
Signed-off-by: Mateus Oliveira <[email protected]>
Introduction of the shell script that allows to run commands inside 1-1 copy of the git oadp folder on the local system. Signed-off-by: Michal Pryc <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpryc 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
bot
added
the
approved
| # so we get the hash of the oadp-operator source code | ||
| pushd "$ABS_OADP_GIT_DIR" | ||
| CURRENT_GIT_HASH=$(git rev-parse HEAD) | ||
| COMMAND_TEMP_DIR=$(TMPDIR="${DEFAULT_TMP_DIR}" mktemp -d -t "${OADP_TMP_DIR_PREFIX}XXXX-${CURRENT_GIT_HASH}") || exit 1 |
There was a problem hiding this comment.
I think -t is deprecated by this command
There was a problem hiding this comment.
I would not worry about that. It's handy and works on MacOS as well. The deprecation help was way over 17 years ago, so I don't think it will be removed any time..
| fi | ||
|
|
||
| # Propagate exit value if was provided | ||
| [ -n "${exit_val}" ] && echo "Exit code: ${exit_val}" && exit "$exit_val" |
There was a problem hiding this comment.
Any reason not to put curly braces around the third instance of exit_val? It looks like they were very carefully used on everything else.
There was a problem hiding this comment.
No reason, small mistake which does not have any implications. The exit_val=$1 makes it a single argument anyway so there is no difference between "$exit_val" and "${exit_val}"
| @@ -0,0 +1,71 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Lev would like to know why you have a HACK in this code.
There was a problem hiding this comment.
The hack folder does already exists in the oadp-operator codebase:
https://github.com/openshift/oadp-operator/tree/master/hack
And it seems natural to put scripts within one folder that may have any name.
However in operator-sdk go-based operators it's a common to use hack/ folder for utility scripts as described in the:
https://sdk.operatorframework.io/docs/overview/project-layout/#golang
We use an absolute path of the place where the script resides, but we need to go one level up to be able to copy/rsync folder structure of the main oadp-operator, that is why we have on line number 4 comment about that.
There was a problem hiding this comment.
I tried testing by running make api-isupdated and I think infinite loop happened. A part of the logs
filename overflows max-path len by 22: .makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe/.makefiletmpdir/oadp_tmp-SGu9-3f22ca6f887188a8df85df6971f29b845e6676fe
Fix quote around TMP_FOLDER_NAME that was passed to rsync and could cause recurisve loop. Also fix some errors reported by shellcheck. Signed-off-by: Michal Pryc <[email protected]>
|
@mpryc: 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-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-robot
added
the
needs-rebase
|
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-sigs/prow repository. |
1 similar comment
|
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-sigs/prow repository. |
|
@mateusoliveira43 is this PR still something we want to address (I can work on rebase) or do we want to go in differeht path ? |
|
yes, we need this to move forward with go mod tidy CI job but I do not understand why we have a folder in gitignore if we are using temporary folders |
In cases things goes wrong the temporary folder may be preserved for debug, and the last thing we want to do is to commit that: |
This is PR similar to #1382, however reimplemented using shell script.
Note I am unable to create commits to 1382, that's why opening new PR so developers can comment which way to go.
Note This is not 100% finished, because it lacks the diff comparison
comparepart, but it can be easily added as part of the shell script and not Makefile if we agree on this implementation.The logic is much simpler, because Makefile is not the right place to include functions from the scripts.