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

Simplying CI #1942

Merged
merged 7 commits into from
Mar 29, 2023
Merged

Simplying CI #1942

merged 7 commits into from
Mar 29, 2023

Conversation

germa89
Copy link
Collaborator

@germa89 germa89 commented Mar 24, 2023

Getting rid of the additional workflow which download artifacts from the main workflow.

It has been proben to not be very reliable.

@germa89 germa89 self-assigned this Mar 24, 2023
@germa89 germa89 added BUG Issue, problem or error in PyMAPDL CI/CD Related with CICD, Github Actions, etc labels Mar 24, 2023
@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Merging #1942 (b06c73a) into main (8e1d1e3) will increase coverage by 81.14%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           main    #1942       +/-   ##
=========================================
+ Coverage      0   81.14%   +81.14%     
=========================================
  Files         0       44       +44     
  Lines         0     7888     +7888     
=========================================
+ Hits          0     6401     +6401     
- Misses        0     1487     +1487     

Copy link
Member

@jorgepiloto jorgepiloto left a comment

Choose a reason for hiding this comment

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

Thanks for opening this, @germa89. It is always great to simplify things.

I would suggest a couple of things to simplify this a bit more:

Consider using a conditional matrix for macos build, see this example.

Unify naming convetion for job steps. Some of them are using "foo-bar" while others follow "foo_bar".

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@germa89 germa89 enabled auto-merge (squash) March 24, 2023 10:10
Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

LGTM - I agree with @jorgepiloto's suggestion, but I understand why you don't want to put those dependencies. So, it's fine by me 😄 In the end, the workflow dependencies are just a suggestion. Plus, this repo is public... so we are not being charged by running the tests

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@RobPasMue RobPasMue dismissed jorgepiloto’s stale review March 29, 2023 20:48

Reasonable point brought up by @germa89 preventing merge. Accepting implementes changes

@germa89 germa89 merged commit 6d4ae80 into main Mar 29, 2023
@germa89 germa89 deleted the ci/fix-merging-to-main branch March 29, 2023 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BUG Issue, problem or error in PyMAPDL CI/CD Related with CICD, Github Actions, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants