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

Topic/gitaction test #126

Merged

Conversation

ohdearaugustin
Copy link
Contributor

Fix #125

@VikashKothary VikashKothary force-pushed the develop branch 2 times, most recently from 24daefe to fb05920 Compare March 4, 2022 22:17
Copy link
Member

@VikashKothary VikashKothary left a comment

Choose a reason for hiding this comment

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

@ohdearaugustin Thanks so much for your contribution.

I expect these pipelines will considerably improve the stability of the software and will really aid me in maintaining the sever so it's much appreciated.

I've left some comments about using the make commands in the CI config. This is mainly done for consistency so that the pipelines will work in a consistently to when the individual scripts are run locally.

@ohdearaugustin
Copy link
Contributor Author

ohdearaugustin commented Mar 10, 2022

I took a look at your comments, so basically you want to use make for the CI. This of course is possible, but I'm absolutely not a fan of it. This is my personal opinion. Why use a build system like poetry and then relay on another system to actually build the everything. Therefore I created the pipeline with the natural tools. I'm not a huge fan of I need 5 extra shell scripts and a make file to build something. This will lead to more complications. In general you want to have one source of true with build system rather the build system does everything or nothing at all. Having a manual way and a build system will lead to clashes in future.

If you insist on doing it with make, i will change. This will also have the effect it will test if the make file is working.

Edit: Grammar/Wording

@VikashKothary
Copy link
Member

VikashKothary commented May 16, 2022

Hi @ohdearaugustin, Thanks for sharing your opinion.

I understand your critique of using make i.e. that it unnecessarily increases the complexity of the build, when the language-specific tools and github action plugins can do the same thing, and I agree it's a valid point.

One of my best practices when using CI/CD pipelines to assist development is to treat it as any other user. This means that any code that can be run by a CI/CD pipeline can be run by a user locally. This is important because it is typically quicker to run steps like linting and testing locally rather than wait for the CI pipeline to run.

Make is my opinionated method of achieving this. And in my experience it prevents a lot of issues in the long run.

That being said, I'm not going to enforce my preference to prevent issues that may never come up. It's also true that your initial code in more in-line with how Github recommends Actions should be used. As such, I'm happy to merge your code in the form you initially recommended.

That being said, yaml is not a place for code. My only request is that you use the bash scripts (with or without make) for the multi-line steps so that developers can use their IDEs with syntax highlighting, linting, etc. Single lines are okay.

@ohdearaugustin
Copy link
Contributor Author

So rebased the PR and included the requested changes. Unittests fail as expected due to #139. Let's me know if you need more changes.

Copy link
Member

@VikashKothary VikashKothary left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for your contribution. This will be massively beneficial for this project.

@VikashKothary VikashKothary merged commit 43437cf into ankicommunity:develop Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add github actions for testing and requirement.txt generation
2 participants