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

Run only docs workflow #2203

Closed
wants to merge 24 commits into from
Closed

Run only docs workflow #2203

wants to merge 24 commits into from

Conversation

pat-s
Copy link
Contributor

@pat-s pat-s commented Aug 12, 2023

fix #2200

Outsourced from #2193

Due to triggers like - ".woodpecker/test.yml" the WFs will still run in this PR and changes will only take effect for a subsequent PR.

Also harmonized the use of &when and &when_paths across the workflows.

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@76f9328). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2203   +/-   ##
=======================================
  Coverage        ?   40.72%           
=======================================
  Files           ?      183           
  Lines           ?    10998           
  Branches        ?        0           
=======================================
  Hits            ?     4479           
  Misses          ?     6168           
  Partials        ?      351           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.woodpecker/docker.yml Outdated Show resolved Hide resolved
@qwerty287 qwerty287 added enhancement improve existing features build CI pipeline related labels Aug 12, 2023
@pat-s
Copy link
Contributor Author

pat-s commented Aug 12, 2023

Now it will run anyhow as I added the .woodpecker path trigger. But we should aim for a test that skips everything after the clone step if the paths don't match.

@pat-s
Copy link
Contributor Author

pat-s commented Aug 12, 2023

https://ci.woodpecker-ci.org/repos/3780/pipeline/7287 looks good. I am now commenting in all the .woodpecker paths again, which will trigger a full rerun in this PR. But for subsequent ones, it should skip them if the paths don't path the config specifications.

@pat-s pat-s requested a review from qwerty287 August 12, 2023 12:56
@pat-s
Copy link
Contributor Author

pat-s commented Aug 12, 2023

Oh no, my auto-formatting made a hell of unrelated changes 🥴️

@pat-s pat-s marked this pull request as ready for review August 12, 2023 12:57
.woodpecker/docker.yml Outdated Show resolved Hide resolved
@@ -18,10 +18,23 @@ variables:
- &platforms_server 'linux/arm/v7,linux/arm64/v8,linux/amd64,linux/ppc64le,linux/riscv64'
- &platforms_preview 'linux/amd64'
- &platforms_alpine 'linux/arm/v6,linux/arm/v7,linux/arm64/v8,linux/amd64,linux/ppc64le'
- &when
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is unused, so we only need path as variable.

@@ -25,7 +25,8 @@ steps:
group: prepare
commands:
- go mod vendor
when: *when
when:
path: *when_path
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change this?
It is currently skipped on builds on main completely, so I'd keep it as it is (same for everything else in this file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I am confused with the different use of *when and *when_path across files. Trying to harmonize...

.woodpecker/test.yml Show resolved Hide resolved
@@ -24,7 +24,8 @@ steps:
commands:
- corepack enable
- pnpm install --frozen-lockfile
when: *when
when:
path: *when_path
Copy link
Contributor

Choose a reason for hiding this comment

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

The when is already part of the variable. You can drop all changes like this in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed all *when to *when_path - OK?

@pat-s
Copy link
Contributor Author

pat-s commented Aug 12, 2023

https://ci.woodpecker-ci.org/repos/3780/pipeline/7293/13 looks good now? 😅 @qwerty287

@pat-s
Copy link
Contributor Author

pat-s commented Aug 12, 2023

@qwerty287 Thanks a lot for your time and patience!

@pat-s
Copy link
Contributor Author

pat-s commented Aug 12, 2023

@anbraten @6543 Unfortunately my editor auto-formatted the file i.e. ' to " and the removal of some whitespace.
Do you mind? Or should I undo all changes again?
Maybe we can also add a linter/editorconfig?

@anbraten
Copy link
Member

I am using prettier to format it.

@pat-s
Copy link
Contributor Author

pat-s commented Aug 12, 2023

Me 2 - and this is what happens if I format the yaml files - so good for you?

In the gitea helm chart we use a yamllint WF for this - maybe we could add that one as well here? https://gitea.com/gitea/helm-chart/src/branch/main/.gitea/workflows/test-pr.yml#L35-L36
It uses this config: https://gitea.com/gitea/helm-chart/src/branch/main/.yamllint


lint-editorconfig:
image: mstruebing/editorconfig-checker
group: test
when:
path: *when_path
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a event: pull_request here too? Just noticed that this also runs on pushes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it to a few more instances. Probably even more refinement possible across all pipelines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm it's actually missing for everything because you changed the when var to only use path filters. Actually every step should have this filter

Copy link
Contributor

Choose a reason for hiding this comment

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

@pat-s see 55134ad for what I mean - and yes, this changes almost nothing from main because the test workflow is already pretty optimized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix it throughout the day.

Overall I think it's important to reduce/avoid unneeded runs to save resources and time - even if it is a bit of tedious small-scale optimization work.

@pat-s pat-s requested a review from qwerty287 August 14, 2023 07:12
Copy link
Contributor

@qwerty287 qwerty287 left a comment

Choose a reason for hiding this comment

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

I'm still not happy with test because the change is not really doing something but complicates the pipeline.
You can directly take the file content of test.yml from 55134ad and apply this here - it contains all the changes but without those that are increasing complexity.

@pat-s
Copy link
Contributor Author

pat-s commented Aug 15, 2023

I don't like the when: *when while other pipelines are using *when_path. To me this increases complexity across pipelines. Which is why I harmonized it.

In some occasions additional modifications to when are needed which are much easier to do when only using a when_path variable and not a when variable.

@pat-s
Copy link
Contributor Author

pat-s commented Aug 16, 2023

I am not sure how/if we can find a consensus here. I am aware that the discussion which is blocking a merge is actually off-topic WRT to the PR title but I feel like aligning variable usage across pipelines is an important one ❓

@6543
Copy link
Member

6543 commented Aug 16, 2023

I'm leaning more towards code deduplication ...

@6543
Copy link
Member

6543 commented Aug 16, 2023

the only things I would accept right away would be the changes to add a path filter to the docker workflow ... the rest is just opinionated formating

as we do with souch bike-shading topics -> the @woodpecker-ci/owner will vote & decide ... cc @anbraten

@pat-s
Copy link
Contributor Author

pat-s commented Aug 17, 2023

the rest is just opinionated formating

Again, that's what prettier did for auto-formatting the file. Either we need a better config or linter. I can remove it again but it's tedious to cherry-pick. That's why it's still in.


# vars used on push / tag events only
- publish_logins: &publish_logins
# Default DockerHub login
- publish_logins: &publish_logins # Default DockerHub login
Copy link
Member

Choose a reason for hiding this comment

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

publish_logins is not default dockerhub login ... the next entry is

@pat-s
Copy link
Contributor Author

pat-s commented Aug 25, 2023

as we do with souch bike-shading topics -> the https://github.com/orgs/woodpecker-ci/teams/owner will vote & decide ... cc @anbraten

Any progress here?

@pat-s
Copy link
Contributor Author

pat-s commented Sep 8, 2023

ping @anbraten @6543

qwerty287 added a commit that referenced this pull request Oct 23, 2023
- removes docker username secret
(#2589)
- fix `when` filters
https://ci.woodpecker-ci.org/repos/3780/pipeline/8907/9
- add path filter to only build docker image if stuff was changed (or
label is added)

closes #2203 
closes #2598
@qwerty287 qwerty287 deleted the run-only-docs branch October 25, 2023 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build CI pipeline related enhancement improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only run docs workflow for docs-only changes
5 participants