Skip to content
This repository has been archived by the owner on Oct 21, 2019. It is now read-only.

Wrong repository and key #280

Merged
merged 2 commits into from
Oct 15, 2018
Merged

Wrong repository and key #280

merged 2 commits into from
Oct 15, 2018

Conversation

juagargi
Copy link
Member

@juagargi juagargi commented Oct 1, 2018

PR quickly created without much testing. It also needs to improve a bit the script for Linux.


This change is Reviewable

@juagargi juagargi requested a review from FR4NK-W October 1, 2018 13:51
Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @juagargi and @FR4NK-W)


vagrant/run.sh, line 52 at r1 (raw file):

            wget -q https://www.virtualbox.org/download/oracle_vbox.asc -O- | sudo apt-key add -
            sudo sh -c 'echo "deb http://download.virtualbox.org/virtualbox/debian xenial contrib" >> /etc/apt/sources.list.d/virtualbox.list'
            sudo apt remove virtualbox virtualbox-5.1 || true

This || true is ineffective. Did you mean to set -e?


vagrant/run.sh, line 65 at r1 (raw file):

                    [Yy]*)
                        sudo bash -c 'echo deb http://vagrant-deb.linestarve.com/ any main > /etc/apt/sources.list.d/wolfgang42-vagrant.list'
                        sudo apt-key adv --keyserver pgp.mit.edu --recv-key AD319E0F7CFFA38B4D9F6E55CE3F3DE92099F7A4

Note: this key server is down (see #267).

Copy link
Contributor

@FR4NK-W FR4NK-W left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @juagargi)


vagrant/run.sh, line 52 at r2 (raw file):

sudo apt remove virtualbox virtualbox-5.1 || true

On line 45 we are checking if virtualbox is installed and if not get to this branch. Why do we need to remove it here?


vagrant/run.sh, line 56 at r2 (raw file):

        fi
        VERS=$(vagrant version | grep "Installed Version:" | sed -n 's/^Installed Version: \(.*\)$/\1/p')
        if verleq 1.9 $VERS; then

Tested the install with Vagrant 1.8.1 (version provided by the package manager on Ubuntu Xenial) which worked fine


vagrant/run.sh, line 68 at r2 (raw file):

--only-upgrade

Do we want to have '--only-upgrade' here? Or do we also install it if it is not yet installed?
Also we have '--no-remove' for the virtualbox install, should we also add it here?

Copy link
Member Author

@juagargi juagargi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @FR4NK-W and @matzf)


vagrant/run.sh, line 52 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

This || true is ineffective. Did you mean to set -e?

Done.


vagrant/run.sh, line 65 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Note: this key server is down (see #267).

Done.


vagrant/run.sh, line 52 at r2 (raw file):

Previously, FR4NK-W wrote…
sudo apt remove virtualbox virtualbox-5.1 || true

On line 45 we are checking if virtualbox is installed and if not get to this branch. Why do we need to remove it here?

Done.


vagrant/run.sh, line 56 at r2 (raw file):

Previously, FR4NK-W wrote…

Tested the install with Vagrant 1.8.1 (version provided by the package manager on Ubuntu Xenial) which worked fine

Done.


vagrant/run.sh, line 68 at r2 (raw file):

Previously, FR4NK-W wrote…
--only-upgrade

Do we want to have '--only-upgrade' here? Or do we also install it if it is not yet installed?
Also we have '--no-remove' for the virtualbox install, should we also add it here?

Done.

Copy link
Contributor

@FR4NK-W FR4NK-W left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matzf)

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@juagargi juagargi merged commit cbf27e2 into netsec-ethz:master Oct 15, 2018
@juagargi juagargi deleted the fix_run.sh branch October 15, 2018 12:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants