-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Update the Vagrantfile to resolve package update/installation issue. #9783
Conversation
522f693
to
8cdc309
Compare
Lots of things are changing here.. I'm wondering what the goal is, or what problem is trying to be solved with these changes. |
9503e50
to
1cfbeb2
Compare
Resolves #9763. |
1cfbeb2
to
ca48b1f
Compare
Manually tested, seem to be working fine:
|
Checking upon boot:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many of these changes seem complex and I'd suggest further discussion. It looks like the referenced issue is cosmetic, and the Vagrantfile still works without any changes. However to clean them up and get the provisioning script to behave better, we could add those Dpkg::Options to Line 24.
I'd also note that the Vagrantfile was only just recently refactored in #8762.
@@ -1,83 +1,108 @@ | |||
# -*- mode: ruby -*- | |||
# vi: set ft=ruby : | |||
|
|||
# Vagrantfile API/syntax version. Don't touch unless you know what you're doing! | |||
VAGRANTFILE_API_VERSION = "2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to remove the version variable?
|
||
# Get the ARCH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to remove the comments? They can help either debug and allow others to contribute.
echo "Downloading go (${GOVERSION}) ..." | ||
wget -P /tmp --quiet "https://storage.googleapis.com/golang/go${GOVERSION}.linux-${ARCH}.tar.gz" | ||
wget -P /tmp -q --no-check-certificate \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should always be checking the certificate. This is a very dangerous flag to set.
config.vm.synced_folder '.', '/opt/gopath/src/github.com/hashicorp/terraform' | ||
|
||
config.vm.provider "docker" do |v, override| | ||
override.vm.box = "tknerr/baseimage-ubuntu-14.04" | ||
if config.vm.respond_to? :use_linked_clone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding sections around linked clones?
|
||
# Setup the GOPATH; even though the shared folder spec gives the working |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all helpful comments, and likely shouldn't be removed.
# parent path to allow subsequent "go get" commands to work. | ||
mkdir -p "$SRCPATH" | ||
chown -R vagrant:vagrant "$SRCPATH" 2>/dev/null || true | ||
# ^^ silencing errors here because we expect this to fail for the shared folder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially this comment, which explains why we're silencing errors.
export DEBIAN_PRIORITY=critical | ||
export DEBIAN_FRONTEND=noninteractive | ||
export DEBCONF_NONINTERACTIVE_SEEN=true | ||
APT_OPTS="--yes --force-yes --no-install-suggests --no-install-recommends" | ||
|
||
APT_OPTIONS='--assume-yes --no-install-suggests --no-install-recommends' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with moving from --force-yes to --assume-yes.
Could we not simply modify this line to the following to resolve the issue? This would preclude the need for major changes to the rest of the file.
APT_OPTS="--assume-yes --no-install-suggests --no-install-recommends -o Dpkg::Options::=\"--force-confdef\" -o Dpkg::Options::=\"--force-confold\""
5e2778e
to
2d908d2
Compare
@cblecker hi there! I am sorry that you have felt so strongly about the changes here. I will remove everything and put only the necessary alternations. |
Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
2d908d2
to
75a614d
Compare
@cblecker hi there. Changes as suggested by you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kwilczynski!
I'm sorry if I came off too strong. I don't mean to shut down all those changes if they had a reason.. it's just the reasons weren't obvious and weren't commented. I actually liked the assume-yes APT_OPTS change (commented again in-line).
That being said, I've tested this pull as-is, and it fixes the issue on my side (Vagrant 1.8, Parallels, bento/ubuntu-14.04 version 2.3.0).
@@ -16,7 +16,7 @@ ARCH="$(uname -m | sed 's|i686|386|' | sed 's|x86_64|amd64|')" | |||
export DEBIAN_PRIORITY=critical | |||
export DEBIAN_FRONTEND=noninteractive | |||
export DEBCONF_NONINTERACTIVE_SEEN=true | |||
APT_OPTS="--yes --force-yes --no-install-suggests --no-install-recommends" | |||
APT_OPTS="--yes --force-yes --no-install-suggests --no-install-recommends -o Dpkg::Options::=\"--force-confdef\" -o Dpkg::Options::=\"--force-confold\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually liked changing --yes --force-yes
to --assume-yes
as it's more graceful, and the man page actually suggests not using force.
Signed-off-by: Krzysztof Wilczynski <krzysztof.wilczynski@linux.com>
@cblecker hi there. Changes applied as per your suggestions. Again, apologies for making you upset. |
@cblecker hi there! I have a question - would you really mind some of the additions to the Vagrantfile? I would like to add the part which enables linked clone. I see why not to? It adds the speed benefits and it should be harmless (given the if-statement there). What do you think? |
LGTM! Thanks for the back and forth coming to an agreement here @kwilczynski and @cblecker :) |
…dentials * upstream/master: (79 commits) update CHANGELOG Update panicwrap to pass through all interrupt signals Gracefully stops on SIGTERM website: update website for conditionals vendor: update HIL with conditionals Keep a consistent provider order. Update CHANGELOG.md provider/aws: Forces the api gateway domain name certificates to recreate the resource (hashicorp#10588) Update CHANGELOG.md provider/aws: FIxed the api_gw_domain_name replace operation (hashicorp#10179) Fixed note formatting Explicitly say `count` is not supported by modules (hashicorp#10553) docs/aws: Fix the discrepencies of the emr_cluster documentation (hashicorp#10578) Update CHANGELOG.md Service role is not updated on AWS for a CodeDeploy deployment group (hashicorp#9866) Update CHANGELOG.md provider/datadog hashicorp#9375: Refactor tags to a list instead of a map. (hashicorp#10570) Update the Vagrantfile to resolve package update/installation issue. (hashicorp#9783) docs/aws: Add iam_server_certificate data source to nav bar (hashicorp#10576) Update CHANGELOG.md ...
* aws/feature/r-instance-net-iface-id: (74 commits) - Properly exercise network_interface_id from AWS SDK - Update Terraform’s documentation Update CHANGELOG.md provider/aws: Forces the api gateway domain name certificates to recreate the resource (hashicorp#10588) Update CHANGELOG.md provider/aws: FIxed the api_gw_domain_name replace operation (hashicorp#10179) Fixed note formatting Explicitly say `count` is not supported by modules (hashicorp#10553) docs/aws: Fix the discrepencies of the emr_cluster documentation (hashicorp#10578) Update CHANGELOG.md Service role is not updated on AWS for a CodeDeploy deployment group (hashicorp#9866) Update CHANGELOG.md provider/datadog hashicorp#9375: Refactor tags to a list instead of a map. (hashicorp#10570) Update the Vagrantfile to resolve package update/installation issue. (hashicorp#9783) docs/aws: Add iam_server_certificate data source to nav bar (hashicorp#10576) Update CHANGELOG.md feat/aws: add iam_server_certificate data source (hashicorp#10558) provider/azurerm: arm_virtual_machine panic fix Update .travis.yml provider/aws: Improved the documentation for EMR Cluster (hashicorp#10563) provider/azurerm: Do not pass an empty string of license_type to AMR VMs (hashicorp#10564) ... # Conflicts: # builtin/providers/aws/resource_aws_instance.go
The dpkg sudo trigger appears to want to run interactively despite the noninteractive debconf setting. This is a problem upstream for vagrant also, see. hashicorp/terraform#9763 Incorporating the recommended fix to Vagrantfile from upstream hashicorp/terraform#9783 Change-Id: I8da8522fc9e80fc3bd268b347a786054ad019170 Signed-off-by: Ray Kinsella <ray.kinsella@intel.com>
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Signed-off-by: Krzysztof Wilczynski krzysztof.wilczynski@linux.com