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

Make use of Poetry parametrized #26

Closed
wants to merge 11 commits into from
Closed

Make use of Poetry parametrized #26

wants to merge 11 commits into from

Conversation

teutoburg
Copy link
Contributor

@teutoburg teutoburg commented Jun 17, 2024

The current "solution" of dealing with repos that use Poetry and also those that don't using two separate branches is not ideal. As we recently saw, any changes to the workflow in general have to be applied to both branches, which is cumbersome. I tried to let the workflow automatically decide which one to use based on the custom repo property that I included in most of our repos some time ago and has a Poetry flag. However, these (rather new in GH) properties are not directly accessible from the workflow, and getting them via the GH API and some creative JSON juggling might be doable but feels super hacky. Rather than that, I decided the best approach would be to add an optional parameter as a Poetry flag. This way, we can use the same workflow for all repos and simply pass a parameter if Poetry should be used.

To allow this change to be possible without breaking anything, I see two options:

  1. Merge this into the poetry branch and make the flag True by default, that way any repos currently using this on the poetry branch without any parameter will get the same behavior as before. Any non-poetry repos are unaffected, but can be slowly switched over to using the poetry branch with the flag set to False (which is somewhat confusing, "use poetry but actually don't", oh well).
  2. Call this branch main and don't merge it anywhere (which would mean closing the PR), and leave the flag to default to False (or not, doesn't actually make a functional difference in this case, as no current workflows would get there without manually changing the branch). Once all non-poetry repos are switched over to using this version as described in 1., the new main branch can be set as the default branch, and the old master discontinued. Similarly, once all poetry-based repos are using the new main branch with the flag True, we can discontinue the current poetry branch.

Which option do you prefer @hugobuddel ?

Copy link

codecov bot commented Jun 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (0702021) to head (3463d40).
Report is 6 commits behind head on poetry.

Additional details and impacted files
@@            Coverage Diff            @@
##            poetry       #26   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines            7         7           
=========================================
  Hits             7         7           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This only occurs in the case of push and PR within the DevOps repo itself.
But it's still necessary to cover this case, and it should use the Poetry-
based variant, as this is based on the DevOps branch that already uses
Poetry. Maybe a cleaner solution could be done with ENV variables? IDK
@teutoburg teutoburg marked this pull request as ready for review June 18, 2024 13:35
@hugobuddel
Copy link
Contributor

Thanks for doing the work! Renaming to main seems more sensible to me. Does require perhaps more repositories to change, but is less surprising.

We could also have different workflows, like tests-poetry.yaml. That is better than having multiple branches w.r.t. updating e.g. the Pyhon version or the action versions. But the poetry flag is also fine with me.

@teutoburg
Copy link
Contributor Author

[...] w.r.t. updating e.g. the Pyhon version or the action versions.

Indeed the python versions were a main motivation for doing this now 🙃

I think with the flag solution we'll have even less places to update things.

@teutoburg
Copy link
Contributor Author

Since we decided to go with option 2. and call this branch main without merging it into the poetry branch, I'm closing this PR.

@teutoburg teutoburg closed this Jun 26, 2024
@teutoburg teutoburg deleted the fh/choosepoetry branch June 26, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants