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

ci: build artifacts on every push and pull #378

Merged
merged 2 commits into from
Feb 4, 2023

Conversation

sansyrox
Copy link
Member

Description

Changing this as I want to build an artifact on every PR. That way it will be easier for us to just download the artifact and test it on our PC.

@sansyrox sansyrox requested a review from AntoineRR January 29, 2023 12:49
@netlify
Copy link

netlify bot commented Jan 29, 2023

Deploy Preview for robyn canceled.

Name Link
🔨 Latest commit 6076bde
🔍 Latest deploy log https://app.netlify.com/sites/robyn/deploys/63dc212f242a4b000952548a

@sansyrox
Copy link
Member Author

Hey @AntoineRR 👋

The rationale behind this is that the top-level tag checking doesn't allow the jobs to run on every PR, and hence we won't be generating build artifacts when required.

@AntoineRR
Copy link
Collaborator

Yes that was the goal of my previous PR actually, but I had no idea you used the artifacts from PR sorry. I found it overkill to build the package for every OS, and every Python version on each PR. I think there are way too many jobs running in PRs (more than 120 in this PR currently!).

Maybe we could only build one package for windows, linux and macos on PRs (based on Python 3.11 for example)? This way we would still have artifacts to test, but only 3 instead of I think 45 currently.

To achieve this, I would create a new workflow (called preview build maybe) that runs on PRs and keep the release workflow as it is.

What do you think @sansyrox ? Let me know if I misunderstood your needs around build artifacts.

@sansyrox
Copy link
Member Author

Maybe we could only build one package for windows, linux and macos on PRs (based on Python 3.11 for example)? This way we would still have artifacts to test, but only 3 instead of I think 45 currently.

This is a great suggestion. Let's build the packages for the latest versions only

@AntoineRR
Copy link
Collaborator

Thanks! Let me know if you need help for this 😄

@sansyrox sansyrox force-pushed the ci/build-artifacts-on-every-merge branch from 5e1d44d to 41a6d41 Compare January 30, 2023 20:53
@sansyrox sansyrox force-pushed the ci/build-artifacts-on-every-merge branch from 41a6d41 to 2e2c72c Compare February 1, 2023 21:18
@sansyrox
Copy link
Member Author

sansyrox commented Feb 1, 2023

@AntoineRR , can you have another review whenever you get the time?

Copy link
Collaborator

@AntoineRR AntoineRR left a comment

Choose a reason for hiding this comment

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

@sansyrox I left you some comments 🙂
Two more things here:

  • I think we could merge the last two jobs, linux and linux-cross together, it should be possible I think
  • Is there an issue with the commit history? I think you merged your changes with one of my previous commit based on the commit message. Maybe you should reopen a new PR if it is too hard to fix?

.github/workflows/python-CI.yml Outdated Show resolved Hide resolved
.github/workflows/preview-deployments.yml Outdated Show resolved Hide resolved
.github/workflows/preview-deployments.yml Outdated Show resolved Hide resolved
.github/workflows/preview-deployments.yml Outdated Show resolved Hide resolved
.github/workflows/preview-deployments.yml Outdated Show resolved Hide resolved
.github/workflows/preview-deployments.yml Outdated Show resolved Hide resolved
.github/workflows/preview-deployments.yml Outdated Show resolved Hide resolved
.github/workflows/preview-deployments.yml Outdated Show resolved Hide resolved
.github/workflows/preview-deployments.yml Outdated Show resolved Hide resolved
.github/workflows/preview-deployments.yml Show resolved Hide resolved
@sansyrox sansyrox force-pushed the ci/build-artifacts-on-every-merge branch 2 times, most recently from 902ff36 to 42cde30 Compare February 2, 2023 20:11
@sansyrox sansyrox force-pushed the ci/build-artifacts-on-every-merge branch 2 times, most recently from 4ece89b to e446939 Compare February 2, 2023 20:18
@sansyrox
Copy link
Member Author

sansyrox commented Feb 2, 2023

Thanks for the suggestions 😄

I think we could merge the last two jobs, linux and linux-cross together, it should be possible I think

These two environments require different logic. It will make the pipeline unnecessarily complex in my opinion.

Is there an issue with the commit history? I think you merged your changes with one of my previous commit based on the commit message. Maybe you should reopen a new PR if it is too hard to fix?

All fixed :D

@sansyrox sansyrox requested a review from AntoineRR February 2, 2023 20:19
Copy link
Collaborator

@AntoineRR AntoineRR left a comment

Choose a reason for hiding this comment

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

There are some minor adjustments needed, but this looks great otherwise, good job! 😉

.github/workflows/preview-deployments.yml Show resolved Hide resolved
.github/workflows/preview-deployments.yml Outdated Show resolved Hide resolved
.github/workflows/preview-deployments.yml Outdated Show resolved Hide resolved
@sansyrox sansyrox force-pushed the ci/build-artifacts-on-every-merge branch from e446939 to 6076bde Compare February 2, 2023 20:46
Copy link
Collaborator

@AntoineRR AntoineRR left a comment

Choose a reason for hiding this comment

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

Good job @sansyrox, LGTM! 👍

@sansyrox sansyrox merged commit e8b2c09 into main Feb 4, 2023
@sansyrox sansyrox deleted the ci/build-artifacts-on-every-merge branch February 4, 2023 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants