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

chore: automate release process (DSP-1492) #52

Merged
merged 9 commits into from Apr 8, 2021

Conversation

@BalduinLandolt
Copy link
Collaborator

@BalduinLandolt BalduinLandolt commented Apr 7, 2021

Resolves: DSP-1492

Questions:

  • in previous version: why the strange dependencies installation?
  • under ubuntu-latest, should one call python or python3, and pip/pip3 respectively?
@BalduinLandolt BalduinLandolt self-assigned this Apr 7, 2021
@BalduinLandolt BalduinLandolt marked this pull request as ready for review Apr 7, 2021
Copy link
Collaborator

@kilchenmann kilchenmann left a comment

LGTM

Since the single steps are in different files, I miss a bit the overview in /actions. But that's only my opinion. Let's see how it works...

Loading

@tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Apr 7, 2021

crazy shit! I'll take a look now tomorrow!

Loading

with:
token: ${{ secrets.GH_TOKEN }}
release-type: python
package-name: gh_action_test
Copy link
Collaborator

@kilchenmann kilchenmann Apr 7, 2021

Choose a reason for hiding this comment

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

Package name is maybe wrong here. This name will be used as prefix in the release

Loading

Copy link
Collaborator Author

@BalduinLandolt BalduinLandolt Apr 7, 2021

Choose a reason for hiding this comment

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

oh, yes, missed that, thanks

Loading

@BalduinLandolt
Copy link
Collaborator Author

@BalduinLandolt BalduinLandolt commented Apr 7, 2021

I divided it into different files in hope the "separation of concerns" would make it simpler...

Also, should I add automated notification too?

Loading

@kilchenmann
Copy link
Collaborator

@kilchenmann kilchenmann commented Apr 7, 2021

I divided it into different files in hope the "separation of concerns" would make it simpler...

No problem. That's fine.

Also, should I add automated notification too?

Yes, if you like. Why not?

Loading

@BalduinLandolt
Copy link
Collaborator Author

@BalduinLandolt BalduinLandolt commented Apr 7, 2021

I divided it into different files in hope the "separation of concerns" would make it simpler...

No problem. That's fine.

It just poses the question, to what extent it should be unified all over the other repos. And if so, how it should be done everywhere.
(I think the PR title test and my modifications to the PR template would add value everywhere. Splitting it into different files is probably a matter of taste.)

Also, should I add automated notification too?

Yes, if you like. Why not?

Done. I hope this will work like this, haven't tested that beforehand.

Loading

# check PR title
- uses: deepakputhraya/action-pr-title@master
with:
regex: '([a-z])+(\(([a-z\-_])+\))?!?: ([a-z])+' # Regex the title should match.
Copy link
Contributor

@tobiasschweizer tobiasschweizer Apr 8, 2021

Choose a reason for hiding this comment

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

Would it make sense to add something like \(DSP-\d+\) to the end of the regex?
By convention, the PR's title should end with "(DSP-123)".

It's is actually missing from this PR's title :-)

Loading

Copy link
Collaborator

@kilchenmann kilchenmann Apr 8, 2021

Choose a reason for hiding this comment

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

Not all PRs have a task on youtrack. So, this can be tricky. And for example, in another repo I had a task of type PM- and not DSP-.

Loading

Copy link
Collaborator Author

@BalduinLandolt BalduinLandolt Apr 8, 2021

Choose a reason for hiding this comment

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

You're right that it's missing here in the PR title... ;-)
But I also think it might become at some point problematic to enforce it. (There are also some tasks with BS-123. I'm not sure this will be the case in the future, so I think it's a bit risky to enforce this strictly.)

Loading

Copy link
Contributor

@tobiasschweizer tobiasschweizer Apr 8, 2021

Choose a reason for hiding this comment

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

maybe make it more general: expect something like: \(.+-\d+\)

Loading

Copy link
Contributor

@tobiasschweizer tobiasschweizer Apr 8, 2021

Choose a reason for hiding this comment

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

however, I leave this up to you

Loading

Copy link
Collaborator Author

@BalduinLandolt BalduinLandolt Apr 8, 2021

Choose a reason for hiding this comment

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

I went even stricter: \([A-Z]+-\d+\)

Do you think it's a problem that I allow only lower case letters in the PR title? Should probably be extended to upper case too, right? (e.g. for abbreviations that often use upper case.)

Loading

Copy link
Contributor

@tobiasschweizer tobiasschweizer Apr 8, 2021

Choose a reason for hiding this comment

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

I went even stricter: \([A-Z]+-\d+\)

you are reading my thoughts :-)

Do you think it's a problem that I allow only lower case letters in the PR title? Should probably be extended to upper case too, right? (e.g. for abbreviations that often use upper case.)

no idea

Loading

Copy link
Collaborator Author

@BalduinLandolt BalduinLandolt Apr 8, 2021

Choose a reason for hiding this comment

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

I'll just add upper case, just to be sure

Loading

Copy link
Contributor

@tobiasschweizer tobiasschweizer Apr 8, 2021

Choose a reason for hiding this comment

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

you can never be too paranoid

Loading

Copy link
Collaborator Author

@BalduinLandolt BalduinLandolt Apr 8, 2021

Choose a reason for hiding this comment

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

we could argue about that ;-)

Loading

@BalduinLandolt BalduinLandolt changed the title chore: automate release process chore: automate release process (DSP-1492) Apr 8, 2021
@BalduinLandolt BalduinLandolt merged commit 6a96eee into main Apr 8, 2021
4 checks passed
Loading
@BalduinLandolt BalduinLandolt deleted the wip/DSP-1492-automate-release branch Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants