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

major deploy tidy #3158

Merged
merged 6 commits into from
Jan 16, 2020
Merged

major deploy tidy #3158

merged 6 commits into from
Jan 16, 2020

Conversation

casperdcl
Copy link
Contributor

@casperdcl casperdcl commented Jan 15, 2020

@casperdcl casperdcl added refactoring Factoring and re-factoring testing Related to the tests and the testing infrastructure build Issues/features related to building dvc install packages. labels Jan 15, 2020
@casperdcl casperdcl requested a review from efiop January 15, 2020 15:17
@casperdcl casperdcl self-assigned this Jan 15, 2020
@casperdcl
Copy link
Contributor Author

casperdcl commented Jan 15, 2020

@efiop I think this is mergeable (the wonderful thing being there's no way to fully test without merging into master and tagging)

@efiop
Copy link
Contributor

efiop commented Jan 15, 2020

@casperdcl Need to be careful about it, I will take a look at it a bit later today. Thank you 🙂

@@ -3,8 +3,16 @@
set -e

if [[ $TRAVIS_OS_NAME == "osx" && $TRAVIS_OSX_IMAGE != "xcode8.3" ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about this osx logic - is it still required?

Copy link
Contributor

Choose a reason for hiding this comment

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

@casperdcl Good catch! Not required anymore, it was a leftover from the older days.

exit 2
fi

# ensure at least one positional arg exists
Copy link

@ghost ghost Jan 15, 2020

Choose a reason for hiding this comment

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

what's the reason behind this check, @casperdcl ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

travis' deploy.file might not exist. This is a convenient way to check

Copy link
Contributor

Choose a reason for hiding this comment

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

@casperdcl Still don't understand what it is for. What is deploy.file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. It is because of globs passed to it.

@efiop
Copy link
Contributor

efiop commented Jan 16, 2020

@casperdcl snap job now takes 19!!! minutes, which is more than 2x longer than other build jobs. Let's remove it and only deploy snap on tags, I don't think anyone needs dev branch snaps anyway.

@casperdcl
Copy link
Contributor Author

That looks like a one-off mistake. Looking at the travis log the job should've taken about 330 seconds (similar to all previous builds). Will try restarting.

@casperdcl
Copy link
Contributor Author

There we go... back down to 6min

@efiop efiop merged commit d1f172f into iterative:master Jan 16, 2020
@efiop
Copy link
Contributor

efiop commented Jan 16, 2020

Thanks! 🙂

@casperdcl
Copy link
Contributor Author

casperdcl commented Jan 16, 2020

Ah I was about to push a little bit of tidying after the reviews. Will do in a new PR (incl fixing any issues if there are any)

@efiop
Copy link
Contributor

efiop commented Jan 16, 2020

@casperdcl Please use WIP prefix to make it clear if a PR is being worked on or not. 🙂

@casperdcl casperdcl deleted the snap-deploy branch January 16, 2020 20:33
casperdcl added a commit to casperdcl/dvc that referenced this pull request Jan 16, 2020
- explains review concerns in iterative#3158
@casperdcl casperdcl mentioned this pull request Jan 16, 2020
2 tasks
@casperdcl
Copy link
Contributor Author

well the 2 review conversations were not yet marked as resolved...

@casperdcl
Copy link
Contributor Author

anyway happily seems to have all worked perfectly. Opened a very tiny #3173.

@efiop
Copy link
Contributor

efiop commented Jan 16, 2020

@casperdcl Yeah, I should've been more careful with that. WIP is appreciated too though 😄 Thanks!

@efiop
Copy link
Contributor

efiop commented Jan 19, 2020

@casperdcl Ok, what I didn't realize is that we broke the staging. Previously we:

  1. checked patch formatting
  2. ran tests
  3. tried to build all needed release packages
  4. built and deployed irreversible packages (e.g. pypi that we can't delete and upload that same version again)

Now we've lost 4 and if one package fails, pypi will be deployed anyways. So if one of the packages finds out that we are missing some import/dependency pypi will have that broken version already available for everyone to install. It is not a giant problem for now, but most likely I will change it back in the future. Please, let's be careful about these next time.

@casperdcl
Copy link
Contributor Author

@efiop the new steps are:

  1. check formatting
  2. concurrently run tests
  3. concurrently build & deploy

The old method was:

  1. check formatting
  2. concurrently run tests
  3. concurrently build
  4. concurrently rebuild & deploy

i.e. I intentionally dropped step 3.

@efiop
Copy link
Contributor

efiop commented Jan 19, 2020

@casperdcl Yep, I understand that. Didn't read your description carefully enough when reviewing. 🙂

@casperdcl
Copy link
Contributor Author

@efiop do you mean you prefer having an extra step? It's slower that way (due to rebuilding for deployment), involves duplication in .travis.yml (so harder to maintain/error prone), and shouldn't be required as the tests are meant to check that everything works.

In fact on occasion one deployment may fail (e.g. due to PyPI servers being temporarily down) which should not affect the other deployments. We can just restart the failing deployment manually.

@efiop
Copy link
Contributor

efiop commented Jan 20, 2020

@casperdcl It being slower is not an issue, as it is only done during a release. Duplication is not nice, I agree, especially with snap, which has quite a bit more involved config than pypi that we used to have.

In fact on occasion one deployment may fail (e.g. due to PyPI servers being temporarily down) which should not affect the other deployments. We can just restart the failing deployment manually.

My concern is that one package will break and while we are fixing it, all other ones will be already released, which might cause a confusion of someone not being able to find a specific type of package.But thinking about it, it is not a big deal these days as the release is not fully automated and neither it will be, because we have to wait manual confirmation from homebrew/conda/choco guys anyway. So let's just keep it as is, it does look nicer now, no doubt about it 🙂 Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues/features related to building dvc install packages. refactoring Factoring and re-factoring testing Related to the tests and the testing infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snap stable channel not updating
2 participants