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

Create test-deploy.yml #530

Merged
merged 17 commits into from
May 17, 2021
Merged

Create test-deploy.yml #530

merged 17 commits into from
May 17, 2021

Conversation

casperdcl
Copy link
Contributor

@casperdcl casperdcl commented May 17, 2021


@casperdcl casperdcl temporarily deployed to acceptance-tests May 17, 2021 07:01 Inactive
@casperdcl casperdcl self-assigned this May 17, 2021
@casperdcl casperdcl added the technical-debt Refactoring, linting & tidying label May 17, 2021
@casperdcl casperdcl temporarily deployed to acceptance-tests May 17, 2021 07:20 Inactive
@0x2b3bfa0 0x2b3bfa0 mentioned this pull request May 17, 2021
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

  • Technically approved: solves the problem.
  • Graphically approved: the dependency graph looks awesome! 😄
  • Philosophically (quasi) approved: minor concerns at Create test.yml #526 (comment).

I personally find merging the two workflows a bit extreme, but everything looks great otherwise. Feel free to merge after reading the linked comment.

@casperdcl
Copy link
Contributor Author

casperdcl commented May 17, 2021

Replied to #526 (comment) there.

I personally find merging the two workflows a bit extreme

I've discussed this with GitHub support and their official stance is that the needs: workflow key is intended for this and unfortunately that means things need to be defined in the same workflow file. They also don't support true YAML so there's no neat way to inject interdependencies.

Do you like graphs? https://github.com/tqdm/tqdm/actions/runs/777757936

@0x2b3bfa0
Copy link
Member

I've discussed this with GitHub support and their official stance is that the needs: workflow key is intended for this and unfortunately that means things need to be defined in the same workflow file. They also don't support true YAML so there's no neat way to inject interdependencies.

So true! Let's hope it changes soon.

Do you like graphs? https://github.com/tqdm/tqdm/actions/runs/777757936

Yum! 😋 That's hard to resist.

The only practical concern I have is that depending on the test job will cause all the release runs (even the cron ones) to require a manual approval through GitHub Environments. If that's good enough for us, :shipit:

@casperdcl
Copy link
Contributor Author

casperdcl commented May 17, 2021

I'm not a fan of the manual approval stuff. Skipping things when there are no credentials (and failing when there's a release with no credentials) seems a better approach to me. Was there some other intention?

@0x2b3bfa0 0x2b3bfa0 changed the title Create test.yml Create test-deploy.yml May 17, 2021
Co-authored-by: Helio Machado <[email protected]>
@casperdcl casperdcl temporarily deployed to test-internal May 17, 2021 09:04 Inactive
@casperdcl casperdcl temporarily deployed to test-internal May 17, 2021 09:06 Inactive
@casperdcl casperdcl temporarily deployed to test-internal May 17, 2021 09:21 Inactive
@0x2b3bfa0
Copy link
Member

cache docker layers rather than rebuild from scratch

Given that the RUN commands are always the same for a major version, how will we know when to use the cached layers and when to refresh them?

cml/Dockerfile

Lines 69 to 74 in 3736d7f

RUN cd /etc/apt/sources.list.d \
&& wget https://dvc.org/deb/dvc.list \
&& apt-get update \
&& apt-get install --yes "dvc=${DVC_VERSION}.*" \
&& apt-get clean \
&& rm --recursive --force /var/lib/apt/lists/*

@casperdcl casperdcl temporarily deployed to test-internal May 17, 2021 09:35 Inactive
@casperdcl casperdcl temporarily deployed to test-internal May 17, 2021 10:30 Inactive
@casperdcl casperdcl temporarily deployed to test-internal May 17, 2021 10:34 Inactive
@casperdcl casperdcl temporarily deployed to test-internal May 17, 2021 12:44 Inactive
@casperdcl casperdcl requested a review from 0x2b3bfa0 May 17, 2021 13:07
@casperdcl casperdcl temporarily deployed to test-internal May 17, 2021 13:20 Inactive
@DavidGOrtega
Copy link
Contributor

This is becoming a monumental PR. Too much to review here.
What it's important is that we need the acceptance test prior to merge and the build. Right now everything looks very nice to me. Im going to do a full trial

@casperdcl casperdcl temporarily deployed to test-internal May 17, 2021 14:43 Inactive
@casperdcl
Copy link
Contributor Author

merging as a "hotfix" (doesn't affect CML JS core) before this bloats more. Also to get the caching into master. Also to lay #526, #528 and #183 to rest.

75% to 50% faster Docker builds! 4min down to <60sec, and 6min down to 3min! ⏩

@casperdcl casperdcl merged commit c0f3158 into master May 17, 2021
@casperdcl casperdcl deleted the test-deploy branch May 17, 2021 15:09
@0x2b3bfa0 0x2b3bfa0 self-assigned this May 18, 2021
@0x2b3bfa0 0x2b3bfa0 mentioned this pull request Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical-debt Refactoring, linting & tidying
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests fail within external PRs
3 participants