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 elasticsearch ci #114

Merged
merged 2 commits into from
Apr 22, 2024
Merged

add elasticsearch ci #114

merged 2 commits into from
Apr 22, 2024

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Dec 26, 2023

Checklist

gurgunday
gurgunday previously approved these changes Dec 26, 2023
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

I'm not sure why we should add this here

@gurgunday
Copy link
Member

Is it because it's not used often?

Just curious

@mcollina
Copy link
Member

It's just used for one module. It should stay in that repo.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 26, 2023

Its just an offer. Probably would remove that github action in the corresponding repo and replace it with the service entry in the github workflow.

Fdawgs
Fdawgs previously approved these changes Jan 24, 2024
@Uzlopak Uzlopak closed this Mar 21, 2024
@gurgunday
Copy link
Member

gurgunday commented Apr 20, 2024

It's just used for one module. It should stay in that repo.

This is also the case with the MongoDB, Postgres, and MySQL workflows

Now, following the same logic, it makes sense to move them to the repos where they're used; however, if we merge #124 and centralize most library CIs too, it could be more efficient in the long term to take it one step further and centralize everything as much as possible

Right now, there is no consistency to when we put a workflow here or to a dedicated repo

So I propose that we:

  • centralize all workflows that follow this generic structure and add one simple thing/feature to it
  • only create a dedicated workflow in a repo if it requires significant modifications to the generic workflow

Benefits:

  • When creating a new repo that will use kafka, mysql, etc., maintainers will know where to look first before thinking about creating a custom workflow
  • When the time comes to modify the generic workflow, it will be easier to also update its flavors

So, in summary, I believe we should merge this and #113, or remove the other workflows from this repo as well...

@gurgunday gurgunday reopened this Apr 20, 2024
@gurgunday gurgunday dismissed stale reviews from Fdawgs and themself via 2c23322 April 21, 2024 09:22
@gurgunday gurgunday requested a review from mcollina April 21, 2024 09:25
@mcollina
Copy link
Member

I would not have this here, but in the elasticsewrch repo

@gurgunday
Copy link
Member

gurgunday commented Apr 21, 2024

Please see the arguments and reasoning at #114 (comment)

In this repo, we already have the MongoDB, MySQL, and PostgreSQL workflows that are only used once — in fastify-mongodb, fastify-mysql, and fastify-posgres respectively, we should remove those from here as well then

But I'm arguing that it's actually easier to manage this way, for instance in #116 all it took was one PR to add BlueOak support to the main workflow and all its flavors, this is just another flavor of that main workflow

If a repo really requires a custom workflow radically different from the main workflow, then I'm not against putting it in its repo, but in general things will become easier to manage for these simple one-feature workflows in my opinion

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@gurgunday gurgunday merged commit 1fd2356 into main Apr 22, 2024
1 check passed
@gurgunday gurgunday deleted the add-elasticsearch branch April 22, 2024 10:47
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