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

Do not overwrite existing files during unzip #47

Merged
merged 1 commit into from
Sep 1, 2020

Conversation

ialidzhikov
Copy link
Member

@ialidzhikov ialidzhikov commented Aug 31, 2020

What this PR does / why we need it:
Staring from v1.92.0 provider-alicloud plugin packages not only the plugin binary, but also the CHANGELOG.md, LICENSE, README.md files.

$ wget https://releases.hashicorp.com/terraform-provider-alicloud/1.94.0/terraform-provider-alicloud_1.94.0_linux_amd64.zip

$ unzip terraform-provider-alicloud_1.94.0_linux_amd64.zip
Archive:  terraform-provider-alicloud_1.94.0_linux_amd64.zip
  inflating: CHANGELOG.md
  inflating: LICENSE
  inflating: README.md
  inflating: terraform-provider-alicloud_v1.94.0 

Trying to bump our provider-alicloud version in #45, fails with:

Creating terraform_0.12.20-bundle2020083107_linux_amd64.zip ...
All done!
Archive:  terraform_0.12.20-bundle2020083107_linux_amd64.zip
  inflating: CHANGELOG.md            
  inflating: LICENSE                 
replace README.md? [y]es, [n]o, [A]ll, [N]one, [r]ename:  NULL
(EOF or read error, treating as "[N]one" ...)
  inflating: terraform               
  inflating: terraform-provider-alicloud_v1.94.0  
  inflating: terraform-provider-aws_v2.68.0_x4  
  inflating: terraform-provider-azurerm_v1.44.0_x4  
  inflating: terraform-provider-google-beta_v3.27.0_x5  
  inflating: terraform-provider-google_v3.27.0_x5  
  inflating: terraform-provider-null_v2.1.2_x4  
  inflating: terraform-provider-openstack_v1.28.0_x4  
  inflating: terraform-provider-packet_v2.3.0_x4  
  inflating: terraform-provider-template_v2.1.2_x4  
The command '/bin/sh -c export TF_VERSION=$(cat /tmp/terraformer/TF_VERSION) &&     export KUBECTL_VERSION=$(cat /tmp/terraformer/KUBECTL_VERSION) &&     apt-get update &&     apt-get install -y unzip &&     mkdir -p /go/src/github.com/hashicorp &&     git clone --single-branch --depth 1 --branch v${TF_VERSION} https://github.com/hashicorp/terraform.git /go/src/github.com/hashicorp/terraform &&     cd /go/src/github.com/hashicorp/terraform &&     go install ./tools/terraform-bundle &&     cd /tmp/terraformer &&     ./scripts/fetch-providers &&     curl -LO https://storage.googleapis.com/kubernetes-release/release/v${KUBECTL_VERSION}/bin/linux/amd64/kubectl &&     chmod +x ./kubectl' returned a non-zero code: 1

With unzip -n we can instruct unzip to do not ask for input (and to do not overwrite existing file) in case of conflict.

Release note:

NONE

Signed-off-by: ialidzhikov <i.alidjikov@gmail.com>
@ialidzhikov ialidzhikov requested a review from a team as a code owner August 31, 2020 16:29
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 31, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 31, 2020
Copy link
Member

@timuthy timuthy left a comment

Choose a reason for hiding this comment

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

Isn't an overwrite -o rather what you want if you bump providers?

@ialidzhikov
Copy link
Member Author

Isn't an overwrite -o rather what you want if you bump providers?

In the above case the README.md coming from terraformer conflicts with README.md that is packaged in the terraform-provider-alicloud plugin. Is there a case that we would need -o instead of -n?
Alternatively I can add a .dockerignore to workaround the issue.

Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added the reviewed/lgtm Has approval for merging label Aug 31, 2020
@timuthy
Copy link
Member

timuthy commented Sep 1, 2020

Isn't an overwrite -o rather what you want if you bump providers?

In the above case the README.md coming from terraformer conflicts with README.md that is packaged in the terraform-provider-alicloud plugin. Is there a case that we would need -o instead of -n?
Alternatively I can add a .dockerignore to workaround the issue.

It was just a thought that corrupted filed would never be overwritten in case you execute the script multiple times (locally), but I'm also okay with this approach.

@rfranzke rfranzke merged commit 903afca into gardener:master Sep 1, 2020
@ialidzhikov ialidzhikov deleted the nit/unzip branch September 1, 2020 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants