FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bug fix: push `:edge` when `VERSION` is updated as well by georglauterbach · Pull Request #3662 · docker-mailserver/docker-mailserver · GitHub

/ docker-mailserver Public

bug fix: push :edge when VERSION is updated as well - #3662

Merged
georglauterbach merged 1 commit into
masterfrom
release/bugfix-edge
Nov 26, 2023
ConversationCommits (1)ChecksFiles changed
Merged

bug fix: push :edge when VERSION is updated as well#3662
georglauterbach merged 1 commit into
masterfrom
release/bugfix-edge

Conversation

georglauterbach commented Nov 26, 2023

Copy link
Copy Markdown
Member

Description

Previously, we did not run the workflow on push on master when a release happened because the push on master is guarded by a check on which files were changed.

With this change, I added VERSION to the list of files to consider when updating :edge.

Superseeds #3659

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas

georglauterbach added area/ci priority/high kind/bug/fix A fix (PR) for a confirmed bug labels Nov 26, 2023
georglauterbach self-assigned this Nov 26, 2023
push :edge when VERSION is updated
3bdd682 
Previously, we did not run the workflow on push on `master` when a
release happened because the push on master is guarded by a check on
which files were changed.

With this change, I added `VERSION` to the list of files to consider
when updating `:edge`.
georglauterbach mentioned this pull request Nov 26, 2023
Closed
12 tasks
georglauterbach enabled auto-merge (squash) November 26, 2023 20:42

georglauterbach commented Nov 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Auto-merge is enabled. I have also manually started a run for the default push action, so an updated :edge image will be available around 10pm (UTC+1) (~ +20min from now on).

georglauterbach added this to the v13.1.0 milestone Nov 26, 2023
casperklein approved these changes Nov 26, 2023
View reviewed changes

casperklein left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

BTW: would it hurt, to just build on all changes regardless of the path?

georglauterbach merged commit 68a43eb into master Nov 26, 2023
georglauterbach deleted the release/bugfix-edge branch November 26, 2023 20:44

georglauterbach commented Nov 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

BTW: would it hurt, to just build on all changes regardless of the path?

Probably not; this is somewhat similar to what I was suggesting in #3659 (comment) too.

georglauterbach commented Nov 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

UPDATE: I guess this PR (unexpectedly 😆) solves the issue quite nicely; we should actually not remove paths: to not unnecessarily build an image (e.g., when the documentation is updated). @casperklein

georglauterbach removed the request for review from polarathene November 26, 2023 20:50
polarathene reviewed Nov 27, 2023
View reviewed changes

polarathene left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We have a better fix IMO, :edge shouldn't care about this at all.

Comment thread .github/workflows/default_on_push.yml
- .gitmodules
- Dockerfile
- setup.sh
- VERSION # also update :edge when a release happens

polarathene Nov 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The comment is unnecessary:

Suggested change
- VERSION # also update :edge when a release happens
- VERSION

VERSION contributes no meaning to :edge. In the past we have once missed bumping VERSION before tagging a release, but since we only publish images other than :edge by tagged commits, we just published a new tag semver-patch release to resolve that.

I'd rather see the VERSION file removed since it's unnecessary to support the functionality it's intended for, but since it the script in all prior releases since v10 will rely on checking VERSION we'd need to keep it around for a while 🤷‍♂️

If the better update check approach is taken, then the addition to build an image when VERSION changes would become redundant:

Suggested change
- VERSION # also update :edge when a release happens

This was referenced Nov 27, 2023
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

polarathene polarathene left review comments
casperklein casperklein approved these changes

Assignees

georglauterbach

Labels

area/ci kind/bug/fix A fix (PR) for a confirmed bug priority/high

Projects

None yet

Milestone

v13.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL