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 taplo CI #4

Merged
merged 14 commits into from
May 15, 2024
Merged

feat: Add taplo CI #4

merged 14 commits into from
May 15, 2024

Conversation

aidan46
Copy link
Contributor

@aidan46 aidan46 commented May 8, 2024

Description

taplo lint and taplo format, in that order, have been add on push and pull request into any branch. Part of #3

`taplo lint` and `taplo format`, in that order, have been add on push
and pull request into any branch.
@aidan46 aidan46 added the devops label May 8, 2024
@aidan46 aidan46 self-assigned this May 8, 2024
@aidan46 aidan46 mentioned this pull request May 8, 2024
8 tasks
jmg-duarte
jmg-duarte previously approved these changes May 8, 2024
Copy link
Collaborator

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

LGTM, but left an idea to consider

.github/workflows/taplo.yaml Outdated Show resolved Hide resolved
aidan46 added 2 commits May 8, 2024 16:03
Add a configuration file for taplo so CI and local can use the same
taplo rules.
Switch from Action for taplo CI to manual install and running. Use only
`taplo format` and remove `taplo lint` check.
th7nder
th7nder previously approved these changes May 8, 2024
Copy link
Contributor

@serg-temchenko serg-temchenko left a comment

Choose a reason for hiding this comment

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

Looks good, but some points should be discussed. Thank you!

.taplo.toml Show resolved Hide resolved
.taplo.toml Outdated Show resolved Hide resolved
.github/workflows/taplo.yaml Outdated Show resolved Hide resolved
.github/workflows/taplo.yaml Outdated Show resolved Hide resolved
@serg-temchenko serg-temchenko added the ready for review Review is needed label May 8, 2024
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@jmg-duarte
Copy link
Collaborator

Actually, I was thinking of having one workflow for the CI, like ci.yaml, to avoid running multiple runners in parallel.

This can be controlled using concurrency. Furthermore, having one workflow doesn't stop runners from running in parallel. If there are no dependencies between jobs, the runners will execute in parallel (source). For example, in this pipeline I wrote in my last job, the build actions (make-*) all ran in parallel, though they are contained in the same workflow.
If you really want sequential, you need to use needs:<last-job> in every job.

Additionally, including all preparation steps such as checking out to the branch, installing extra dependencies, caches, and sharing the context between workflows would make it unnecessarily complex and introduce a lot of duplication that needs to be maintained in multiple places.

It doesn't work this way for the preparation steps and dependencies. The "work unit" in GitHub Actions is the job, which gets picked up by runners, whether they are in the same workflow or not it doesn't matter, you'll still need to always set them up with extra dependencies unless you're running self-hosted runners.

I agree with the context and cache point though, especially because GitHub doesn't have a good way to orchestrate multiple workflows, but keep in mind that a big CI pipeline may work against us in terms of time spent. Regardless, we can always change it as the project evolves.

@aidan46 aidan46 changed the base branch from main to feat/3/CI-setup May 10, 2024 09:25
jmg-duarte
jmg-duarte previously approved these changes May 10, 2024
Copy link
Collaborator

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

LGTM

@aidan46
Copy link
Contributor Author

aidan46 commented May 10, 2024

I set the target branch to feat/3/CI-setup to merge all the CI into before merging main. LMK if you guys prefer that or just merge straight to main.

th7nder
th7nder previously approved these changes May 10, 2024
@jmg-duarte
Copy link
Collaborator

I set the target branch to feat/3/CI-setup to merge all the CI into before merging main. LMK if you guys prefer that or just merge straight to main.

This makes sense to me because there's a single tracking issue, once the other branch gets merged that works out.

But it will depend on how linear you guys want the history to be. Personally, I prefer linear histories (to the point that I prefer rebase over merge for my projects)

@th7nder
Copy link
Contributor

th7nder commented May 10, 2024

I set the target branch to feat/3/CI-setup to merge all the CI into before merging main. LMK if you guys prefer that or just merge straight to main.

I was going through the GitFlow and didn't see any recommendations for this at a glance (we use this model apparently).
If a change is working, is kinda self-contained and thoroughly tested I don't see a reason for it not to be merged into develop.
It can be useful for other people, who create their own branches off-of develop.

@serg-temchenko serg-temchenko changed the base branch from feat/3/CI-setup to feat/1/project-setup May 10, 2024 16:44
@aidan46 aidan46 requested a review from serg-temchenko May 13, 2024 11:35
@aidan46 aidan46 linked an issue May 13, 2024 that may be closed by this pull request
8 tasks
@aidan46 aidan46 removed a link to an issue May 13, 2024
8 tasks
serg-temchenko
serg-temchenko previously approved these changes May 14, 2024
Copy link
Contributor

@serg-temchenko serg-temchenko left a comment

Choose a reason for hiding this comment

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

Waiting for PR #5 to be merged first so that we have a clean diff.

Base automatically changed from feat/1/project-setup to develop May 14, 2024 16:41
@serg-temchenko serg-temchenko dismissed stale reviews from th7nder, jmg-duarte, and themself May 14, 2024 16:41

The base branch was changed.

@aidan46 aidan46 merged commit 6ea4d8a into develop May 15, 2024
2 checks passed
@aidan46 aidan46 deleted the feat/taplo-ci branch May 15, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants