search

knative / hack

Knative common scripts.
Apache License 2.0
18 stars 64 forks source link

:gift: Source embedded hack scripts #222

Closed cardil closed 1 year ago

cardil commented 2 years ago

Changes

/kind enhancement

Fixes #221

knative-prow[bot] commented 2 years ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

cardil commented 2 years ago

/test all

cardil commented 2 years ago

This works pretty well, as can be seen in example usage PR: https://github.com/knative-sandbox/kn-plugin-event/pull/220. See example build 1568204500513067008.

I have some doubts:

  1. The extracted files are not getting removed when the scripts end. On a developer's box, this might be a nuisance. After a while, people would have a large amount of hack scripts extracted to their /tmp directory. I wonder, is that a problem? It's straightforward to clean up - rm -rf /tmp/knative.*.
  2. The hack scripts are getting extracted to ARTIFACTS subdirectory. So, the scripts get saved as Prow build artifacts. Maybe better would be to save them to an unrelated temp directory. Or, perhaps we like to save them there, to allow for easier debugging. IDK.
  3. I think it would be a good idea to set --verbose flag automatically on Prow build. This could be a follow-up, OFC.

/assign @kvmware /assign @upodroid /cc @mgencur

krsna-m commented 2 years ago

I have some doubts:

  1. I don't like the idea of lingering files. I think a tmp directory that won't be used normally should be used so that you can rm -rf each pass to ensure that we are working with a clean env.
  2. I think that should be a flag. Normally adding to artifacts is not desirable imo, but I see the case of someone wanting to debug and passing a flag to include them.
cardil commented 2 years ago

Re @kvmware:

  1. I don't like the idea of lingering files. I think a tmp directory that won't be used normally should be used so that you can rm -rf each pass to ensure that we are working with a clean env.

I don't like untidy programming as well. The problem is that it's hard to know, is it safe already to delete those files. We execute those scripts in all kinds of different ways. We might try adding some trap to remove files at EXIT signal, but it might backfire in some cases.

The other thing is that, most of the runs are leaving report files either way. Personally, I often use export ARTIFACTS=/tmp/knative-junk before doing any development, so everything is contained in a single "junk" folder. That's easy to clean up afterwards.

Also, /tmp should be cleared with reboot, and those hack scripts are only a couple KiB in size. People would need to run literally millions of times, without reboot, to have real impact on a modern laptop drive.

  1. I think that should be a flag. Normally adding to artifacts is not desirable imo, but I see the case of someone wanting to debug and passing a flag to include them.

Fair enough. This sounds like a good idea to follow up in separate PR, later.

krsna-m commented 2 years ago

lgtm with followup pr/issue for discussed above.

upodroid commented 2 years ago

lgtm

can you try and do some thing like /tmp/HASH/knative and delete /tmp/HASH/knative after each run ?

krsna-m commented 2 years ago

lgtm

can you try and do some thing like /tmp/HASH/knative and delete /tmp/HASH/knative after each run ?

Something like that would be desirable and would be fulfil the ask I had here: I don't like the idea of lingering files. I think a tmp directory that won't be used normally should be used so that you can rm -rf <dir_name> each pass to ensure that we are working with a clean env.

cardil commented 2 years ago

lgtm

can you try and do some thing like /tmp/HASH/knative and delete /tmp/HASH/knative after each run ?

Let me repeat once again. The problem is that it's hard to know, is it safe already to delete those files. We execute those scripts in all kinds of different ways.

When we delete those files at EXIT, they might be read by the parent script even when we push the scripts to a unique folder each source time.

Also, even if that is not the case currently, people could easily introduce such erroneous behavior in the future. And that would be very hard to figure out.

cardil commented 2 years ago

On yesterday's WG meeting, we discussed this with @upodroid.

TL;DR: I'm proposing to extract script in a single location, overwriting any previous files, if any.

Script deletion at end of run is technically hard to do and might be dangerous. It is hard to do because the usage looks like:

source "$(go run knative.dev/hack/cmd/script release.sh -v)"

The script Go binary only prints file path on Stdout. To introduce removal, we would need to modify hack scripts on the fly, adding a trap in the appropriate place. This might be hard to understand, and people might want to have access to those files after the execution for inspection and debugging.

Also, even if we add a working script's removal, it may backfire quite easily. The scripts could invoke themselves, could run subshells, daemons. The removal could be executed too early, while still some scripts are executing. This might be very hard to debug...

That's why I'm proposing to extract hack scripts to a single location, in the temporary directory. Extracted files will overwrite any previous files, and wouldn't be deleted at the end of the script. This approach address all the issues I mentioned above.

cardil commented 2 years ago

:hand: This PR should be ready for re-review.

I changed:

krsna-m commented 2 years ago

lgtm :+1:

cardil commented 1 year ago

I'd like to test this before merging

/hold

cardil commented 1 year ago

The testing PR knative-extensions/kn-plugin-event#307 works well. We can go ahead with merging this.

/hold cancel

knative-prow[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil, upodroid

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: - ~~[OWNERS](https://github.com/knative/hack/blob/main/OWNERS)~~ [cardil,upodroid] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment