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

feat: Add Github Actions #41

Merged
merged 17 commits into from
Aug 7, 2024
Merged

feat: Add Github Actions #41

merged 17 commits into from
Aug 7, 2024

Conversation

maiquanghiep
Copy link
Collaborator

No description provided.

Copy link
Member

@filippos47 filippos47 left a comment

Choose a reason for hiding this comment

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

Great effort!
Overall, a bit frustrating that we need to introduce a separate tagging convention just for a single service type (nodejs). Are there any alternatives that do not affect the tagging? @jrwbabylonlab

This also breaks the automated devnet deployment for the simple-staking service on babylon-service-deployment:

https://github.com/babylonlabs-io/babylon-service-deployment/blob/330243364dd7f984e337bd00069b50c87423c5e2/.github/workflows/commit-schedule-checker.yml#L82

.github/workflows/publish.yaml Show resolved Hide resolved
@jrwbabylonlab
Copy link
Collaborator

Great effort! Overall, a bit frustrating that we need to introduce a separate tagging convention just for a single service type (nodejs). Are there any alternatives that do not affect the tagging? @jrwbabylonlab

This also breaks the automated devnet deployment for the simple-staking service on babylon-service-deployment:

https://github.com/babylonlabs-io/babylon-service-deployment/blob/330243364dd7f984e337bd00069b50c87423c5e2/.github/workflows/commit-schedule-checker.yml#L82

Not i'm aware of. FE deployment is always quite different to others as it does not dynamically accept envs.
All code/env need to be ready at build time as webpack will bundle them together

Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

lgtm, although this again introduces coupling between a service repository and an internal deployment. No problem with merging this now, but we should remove the tight coupling from here.

.github/workflows/publish.yaml Show resolved Hide resolved
.github/workflows/publish.yaml Outdated Show resolved Hide resolved
Copy link
Member

@filippos47 filippos47 left a comment

Choose a reason for hiding this comment

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

As we discussed offline, we can merge this for now.
The next iteration involves templatizing the extra publish logic into the reusable publish workflow.

@maiquanghiep maiquanghiep merged commit 3b4bbb7 into main Aug 7, 2024
1 check passed
@maiquanghiep maiquanghiep deleted the hiep/add-github-actions branch August 7, 2024 09:38
@jrwbabylonlab
Copy link
Collaborator

@maiquanghiep it's best to squash merge. otherwise we end up with too many random commits

@maiquanghiep
Copy link
Collaborator Author

@jrwbabylonlab Sorry my bad, selected squash but forgot to do the same after reloading the page

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