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

fix #16 #18

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@Mte90
Contributor

Mte90 commented Oct 1, 2018

No description provided.

run.sh Outdated
counter=$((counter+1))
if [ ! -d "vvv" ]; then
echo -e "\033[0;32m${counter}/${total} Cleaning up build and vvv folders\033[0m"
rm -rf build vvv

This comment has been minimized.

@tomjn

tomjn Oct 1, 2018

Member

This will fail as it only runs if the vvv folder doesn't exist, so there's nothing to remove

This comment has been minimized.

@Mte90

Mte90 Oct 2, 2018

Contributor

I will remove it

run.sh Outdated
rm -rf build vvv
mkdir -p build vvv
counter=$((counter+1))
fi

This comment has been minimized.

@tomjn

tomjn Oct 1, 2018

Member

there's no else statement with a git pull. In which case if stuff was pulled down, we'd need to reprovision afresh and regenerate the box file

run.sh Outdated
echo -e "\033[0;32m${counter}/${total} Destroying temporary VM\033[0m"
VVV_SKIP_LOGO=true vagrant destroy --force
counter=$((counter+1))
if [ ! -f "../build/vvv-contribute.box" ]; then

This comment has been minimized.

@tomjn

tomjn Oct 1, 2018

Member

If a bad box gets generated, or a newer VVV comes out, then the script won't rebuild the box as the file will already be there

This comment has been minimized.

@Mte90

Mte90 Oct 2, 2018

Contributor

I think that can I can improve the check there based on the filesize as example but usually the issues are not on generating the box but doing the provision.

This comment has been minimized.

@Mte90

Mte90 Oct 2, 2018

Contributor

maybe we can check the date, if is old go on but it is the same day of today will ignore that step.

@tomjn

This comment has been minimized.

Member

tomjn commented Oct 1, 2018

Some of this is good, but it's too geared towards the scenario of provisioning only got halfway through. Even in that scenario you still have to reprovision from scratch, and this doesn't do a lot to help with that as downloading files and moving them around is the cheapest part of the process

But more importantly, I have to delete the entire folder and reclone it if I want to create a new VVV contributor day setup, else it'll just generate the same box as the previous time when the script is ran.

Instead, if you need to run provisioning again because it failed the first time, we should fix that instead. If it's speed you're after, then separating out the meta environment into separate chunks would be better.

Otherwise, the middle section of the script would need rewriting to work on the assumption of a persistent VVV install, not a temporary one

Show resolved Hide resolved run.sh
@Mte90

This comment has been minimized.

Contributor

Mte90 commented Oct 2, 2018

I am trying the script so I will check all the cases, I wanted just to be ready to speed up.

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Oct 14, 2018

I will wait the merge of #26 before resolve the conflicts.

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Oct 14, 2018

aligned :-)

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Oct 18, 2018

I think that is ready for the merge

@tomjn

This comment has been minimized.

Member

tomjn commented Oct 18, 2018

I'm not sure, I need to spend some time really thinking through the mechanics of this. It would have been easier if each part was in its own PR rather than all together, e.g. the downloading of the installers is an easy win, but the parts where it provisions VVV and pulls down a copy I need to think through the mechanics as those could get borked. The current master has the benefit that if the VVV gets royally f***d it doesn't matter because you run it again and it flushes all the borked stuff out and puts things in fresh

@tomjn tomjn added the enhancement label Oct 18, 2018

@tomjn

This comment has been minimized.

Member

tomjn commented Oct 18, 2018

Additionally, a lot of the time spent in the script is in the provisioning of the meta environment. Speeding that up or parallelising steps in it would speed things up immensely for all involved

@Mte90

This comment has been minimized.

Contributor

Mte90 commented Oct 19, 2018

Maybe we can reject this and do another script that is only for the vagrant part.
run.sh script will execute that script and also the rust of stuff so we don't need this kind of checks.
If someone need to update vagrant can use the other one or if has problems can execute it again.

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