Skip to content

Conversation

@abserari
Copy link
Contributor

@abserari abserari commented Mar 4, 2023

Thank you for contributing to Velero!

Please add a summary of your change

Unfortunately, I find the tiltfile is broken by the change https://github.com/vmware-tanzu/velero/tree/fc0c4703950452f035788b0831ca2c2fd4968a59, and I find that I should update again in this commit, for fixing the tilefile. But I don't know if have a better way to build restic files by my changes. Welcome any suggestions.

    cmd = 'cd ' + '.' + ';rm -rf ../restic; BIN=velero GOOS=linux GOARCH=amd64 GOARM="" RESTIC_VERSION=0.13.1 OUTPUT_DIR=../velero/_tiltbuild/restic ./hack/build-restic.sh',

Does your change fix a particular issue?

Fixes #5948

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@abserari
Copy link
Contributor Author

abserari commented Mar 4, 2023

/kind changelog-not-required

@github-actions github-actions bot added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Mar 4, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #5949 (ac25721) into main (94fec66) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #5949      +/-   ##
==========================================
+ Coverage   39.94%   39.98%   +0.03%     
==========================================
  Files         254      254              
  Lines       22361    22361              
==========================================
+ Hits         8932     8940       +8     
+ Misses      12773    12766       -7     
+ Partials      656      655       -1     
Impacted Files Coverage Δ
pkg/restore/restore.go 64.73% <0.00%> (+0.55%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

sseago
sseago previously approved these changes Mar 8, 2023
Signed-off-by: DingRui Yang <[email protected]>
local_resource(
"restic_binary",
cmd = 'cd ' + '.' + ';mkdir -p _tiltbuild/restic; BIN=velero GOOS=linux GOARCH=amd64 GOARM="" RESTIC_VERSION=0.13.1 OUTPUT_DIR=_tiltbuild/restic ./hack/build-restic.sh',
cmd = 'cd ' + '.' + ';rm -rf ../restic; BIN=velero GOOS=linux GOARCH=amd64 GOARM="" RESTIC_VERSION=0.13.1 OUTPUT_DIR=../velero/_tiltbuild/restic ./hack/build-restic.sh',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we'd want to run rm -rf without confirmation.

If your $PWD is /git/velero and you have /git/restic as well wouldn't this command delete restic dir outside of velero repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you're right about the concern, I don't think rm -f should be in this file when it removes outside files. Maybe we need to change the restic download shell script and let all the thing under the Velero repo is good.

The origin reason is someone committed ./hack/build-restic.sh, and don't notice the file was also used by tilt up, so broke the restic download behavior in tilt up.

I suggest creating a new hack script for tilt to download the restic file, although there would be some redundant code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

could nest this under Makefile target ./path/to/restic/code:
and then add this path to gitignore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tilt up failed because the restic binary files output other location.

4 participants