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

ci: setup initial release process #23

Merged
merged 9 commits into from
May 1, 2024
Merged

Conversation

beeme1mr
Copy link
Member

This PR

  • adds pr linting GH action
  • adds release please GH action

Notes

This won't actually publish the Gem but includes all the plumbing.

Follow-up Tasks

How to test

We'll do it live!

Signed-off-by: Michael Beemer <[email protected]>
.github/workflows/release-please.yaml Outdated Show resolved Hide resolved
.github/workflows/release-please.yaml Outdated Show resolved Hide resolved
.release-please-manifest.json Outdated Show resolved Hide resolved
release-please-config.json Outdated Show resolved Hide resolved
release-please-config.json Outdated Show resolved Hide resolved
Signed-off-by: Michael Beemer <[email protected]>
Copy link
Contributor

@alxckn alxckn left a comment

Choose a reason for hiding this comment

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

Not familiar with release-please, what is the intended workflow? For release please to maintain a PR as described on its repo that once merged will proceed with the release?

Will it require that each provider gets released at the same time?

.github/workflows/release-please.yaml Outdated Show resolved Hide resolved
Copy link
Member

@maxveldink maxveldink left a comment

Choose a reason for hiding this comment

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

@beeme1mr I'm glad this is a forcing function to address the naming inconsistencies we have. Here's some thoughts:

  • Conventionally in Ruby, you'll see snake_case files map to PascalCase constants. So, if we want the OpenFeature to be our top-level module, you'd expect every path to be open_feature. We've been talking about this in this issue on the SDK. My preference is we do that here.
  • This runs into an issue with the SDK artifact we're currently releasing. openfeature-sdk would result in an expected constant of Openfeature::Sdk. Most folks will conventionally know that Sdk should be inflected to SDK, so that's fine, but the top-level module is still named unexpectedly. I have thought that it might make sense to rename the primary package to open_feature before we release 1.0 (also dropping SDK because I don't think it adds value for us), but I didn't know how much will there would be for that.
  • Bringing that all into these gems, I think my desired naming would be open_feature-meta_provider and open_feature-flag_d (of open_feature-flag_d_provider if we're only shipping a provider). That matches gem naming conventions where the - indicates that something is modifying/plugging into another gem and has the expected casing.
  • I am but one voice in this, and happy to have the discussion. There's plenty of non-conventional gem names/module pathing and it's fine as long as you explain usages in your README.

Signed-off-by: Max VelDink <[email protected]>
@maxveldink
Copy link
Member

@beeme1mr I made some modifications to the naming for the meta_provider (and I'll have some follow-on changes in the ruby-sdk). I like the place we ended up 👍🏻

@beeme1mr
Copy link
Member Author

@beeme1mr I made some modifications to the naming for the meta_provider (and I'll have some follow-on changes in the ruby-sdk). I like the place we ended up 👍🏻

Awesome, looks good to me! Are we good to merge? If this works, it should create release PRs, and merging those PRs should do everything except publish the Gem. If that goes smoothly, I think it should be pretty straightforward to finish up the publishing step.

@maxveldink
Copy link
Member

Yep! I'll try and merge this in first thing tomorrow.

@maxveldink maxveldink merged commit 278c956 into main May 1, 2024
8 checks passed
@maxveldink maxveldink deleted the setup-release-process branch May 1, 2024 11:20
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.

4 participants