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

[#13] make CI jobs more consistent with open-api-framework related projects #129

Merged
merged 20 commits into from
Oct 4, 2024

Conversation

SonnyBA
Copy link
Contributor

@SonnyBA SonnyBA commented Sep 17, 2024

Fixes maykinmedia/open-api-framework#13

Changes

Reuses open-api-workflows workflows to make CI jobs consistent across the different projects.

@SonnyBA SonnyBA self-assigned this Sep 17, 2024
@SonnyBA SonnyBA added the enhancement New feature or request label Sep 17, 2024
@SonnyBA SonnyBA marked this pull request as ready for review September 17, 2024 12:57
Copy link
Contributor

@Coperh Coperh left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

Will you be merging the open-api-workflows to master before merging?

.github/workflows/generate-sdks.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@annashamray annashamray left a comment

Choose a reason for hiding this comment

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

Looks nice, just some minor remarks.

requirements/base.txt Outdated Show resolved Hide resolved
@@ -38,5 +38,6 @@ pip-compile \
--no-emit-index-url \
--output-file requirements/dev.txt \
"$@" \
requirements/ci.txt \
requirements/base.txt \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this change needed? I thought it would be faster to reuse already compiled ci.txt here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could revert this and include pytest in the dev requirements for other projects 🤔 (to keep the requirements similar to the other projects). pytest is needed for projects which need to generate documentation through github actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, I saw there were some dependencies in test-tools.in which were not used for development, see c5fd869#diff-341d4064347b7e95989c82d7e176e19934bea3913d05bf0b244057aaf3b3e567L3

.github/workflows/code-quality.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@annashamray annashamray left a comment

Choose a reason for hiding this comment

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

Looks good! Could you make bin/compile_dependecies.bat and bin/compile_dependecies.sh consistent with each other?
Otherwise let's proceed with this change

Comment on lines +26 to +28
requirements/base.txt^
requirements/test-tools.in^
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this file should have the same changes as bin/compile_dependencies.sh

@SonnyBA SonnyBA force-pushed the feature/13-consistent-ci-configuration branch from c5fd869 to a8472f0 Compare October 4, 2024 09:00
@SonnyBA
Copy link
Contributor Author

SonnyBA commented Oct 4, 2024

Depends on #133

@SonnyBA SonnyBA force-pushed the feature/13-consistent-ci-configuration branch from 5d43ebb to 638adf4 Compare October 4, 2024 09:46
@SonnyBA SonnyBA merged commit e19d871 into master Oct 4, 2024
16 checks passed
@SonnyBA SonnyBA deleted the feature/13-consistent-ci-configuration branch October 4, 2024 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make CI pipelines the same across projects
4 participants