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

add deployment scripts #31

Merged
merged 10 commits into from
Feb 22, 2024
Merged

add deployment scripts #31

merged 10 commits into from
Feb 22, 2024

Conversation

rkaminsk
Copy link
Member

@rkaminsk rkaminsk commented Feb 21, 2024

TODO

  • refine DEPLOYMENT.md
  • consider running tests before deploy

@rkaminsk rkaminsk mentioned this pull request Feb 21, 2024
@rkaminsk rkaminsk mentioned this pull request Feb 21, 2024
@rkaminsk
Copy link
Member Author

Just going to merge to get it off the table. If further changes are required, we can just open a PR.

@rkaminsk rkaminsk merged commit ed4673a into master Feb 22, 2024
3 checks passed
@rkaminsk rkaminsk deleted the feature/deploy branch February 22, 2024 11:25
@@ -1,6 +1,6 @@
MIT License

Copyright (c) 2024 <author>
Copyright (c) 2024 Mister Fillname
Copy link
Member

@MaxOstrowski MaxOstrowski Feb 22, 2024

Choose a reason for hiding this comment

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

Miss Fillname ? @rkaminsk

Copy link
Member

Choose a reason for hiding this comment

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

How about Mx. Fillname ?

Copy link
Member

@sthiele sthiele Feb 22, 2024

Choose a reason for hiding this comment

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

But I think <author> was also okay.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's intended. It will be replaced by the init script.

Copy link
Member Author

Choose a reason for hiding this comment

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

With <author> it might still have worked. But for example with <email> the tests failed because <email> did not have the proper form.

The only reason why I did the change was to give everything a form as it would occur in a real project to be able to test the project. It was not even intended as a joke. I just wanted to have Fillname in here to have unique token to replace. We can also use Fillname Author or similar if you think that is better.

Copy link
Member

@MaxOstrowski MaxOstrowski Feb 22, 2024

Choose a reason for hiding this comment

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

Ah, and now I also understand your comment as you wanted the template to be fit the regular expressions.
But I still don`t get why this could be useful. To test the project-template one would simply choose 4 fake things and then test for them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, and now I also understand your comment as you wanted the template to be fit the regular expressions. But I still don`t get why this could be useful. To test the project-template one would simply choose 4 fake things and then test for them?

If we use these values, the test workflow will fail because the project template cannot be installed (which is required for testing). The test workflow also runs for the template.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could make the values a bit more consistent:

[email protected]
Author Fillname
https://fillname.org/
fillname

Copy link
Member

Choose a reason for hiding this comment

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

OK, I tried to replace it the way I suggested in my own project and could see the error you meant. Basically the pyproject.toml isnt able to install a project called "".
https://github.com/MaxOstrowski/test-template/

Not sure this is a bad thing though. It prevents the user from using the project without executing init.py first.
On the other hand it's not looking too nice if the CI test in the template repo fail.
Maybe your more consistent set of names is a good tradeoff.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe your more consistent set of names is a good tradeoff.

I think it is. Having the ci tests running, will ensure that we do not have unnecessary errors in the python code as we continue extending the template.

We could also extend the CI tests to run the init script but that would be a bit cumbersome to implement.

The current approach is an easy workaround with somewhat curios placeholders that is otherwise nice and simple.

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.

3 participants