Skip to content

setup.sh: docker_container first, then fall back to docker_image#2134

Merged
georglauterbach merged 5 commits intodocker-mailserver:masterfrom
NorseGaud:docker-container-first
Sep 6, 2021
Merged

setup.sh: docker_container first, then fall back to docker_image#2134
georglauterbach merged 5 commits intodocker-mailserver:masterfrom
NorseGaud:docker-container-first

Conversation

@NorseGaud
Copy link
Copy Markdown
Member

@NorseGaud NorseGaud commented Aug 14, 2021

Description

Here is an original discussion about this: #1874 (comment)

Then there is #2080 (comment): As seen in the comment thread, a user experienced confusion and didn't realize USE_CONTAINER=true or -c was necessary. With my current knowledge I can't see any reason why we wouldn't use the container by default and if it exists, fall back to using the default image/tag when it doesn't.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change that does improve existing functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@NorseGaud NorseGaud self-assigned this Aug 14, 2021
@NorseGaud NorseGaud added meta/wip area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Aug 14, 2021
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Comment thread setup.sh Outdated
@NorseGaud
Copy link
Copy Markdown
Member Author

Very interesting -- a test failed, but I can't reproduce it locally. Seems like there is some sort of flakeyness

@georglauterbach
Copy link
Copy Markdown
Member

I've restarted the test suite. Let's see.

@NorseGaud
Copy link
Copy Markdown
Member Author

@wernerfred any way that I can get access to re-run or cancel actions stuff? I think I already asked that, but I wanted to double check.

@wernerfred
Copy link
Copy Markdown
Member

wernerfred commented Aug 17, 2021

@wernerfred any way that I can get access to re-run or cancel actions stuff? I think I already asked that, but I wanted to double check.

if you did so, i missed it!

imho nothing speaks against "promoting" you to be a maintainer which would include that permission. Another way would be to create a new team with write permission to the repository (kind of between triage and maintainer from a permission perspective) if you prefer not to activly contribute as a maintainer but are slowed down in your contributions through the current permission level.

image

cc @georglauterbach

@NorseGaud
Copy link
Copy Markdown
Member Author

NorseGaud commented Aug 17, 2021

@wernerfred any way that I can get access to re-run or cancel actions stuff? I think I already asked that, but I wanted to double check.

if you did so, i missed it!

imho nothing speaks against "promoting" you to be a maintainer which would include that permission. Another way would be to create a new team with write permission to the repository (kind of between triage and maintainer from a permission perspective) if you prefer not to activly contribute as a maintainer but are slowed down in your contributions through the current permission level.

image

cc @georglauterbach

Maintainer works for me. I don't see myself ever not using this repo to host my personal mail server which means I'll always be contributing. :)

I'll probably be doing some stress testing on the test suite too since the more I use it the more I see random failures that don't reoccur, so this is perfect time to be talking about this.

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Aug 17, 2021

@wernerfred I'm fine with @NorseGaud becoming a maintainer :)

We should however keep the "write" permission level in mind - maybe it could come in handy some time in the future 👍🏼

@wernerfred
Copy link
Copy Markdown
Member

All set 🚀

@NorseGaud
Copy link
Copy Markdown
Member Author

All set 🚀

Gracias, bros!

@NorseGaud
Copy link
Copy Markdown
Member Author

Tests are definitely flakey, and it's usually the same ones. I'll keep stress testing them and see if I can figure out why it's failing.

@NorseGaud
Copy link
Copy Markdown
Member Author

Tested just the set of tests around what is failing and can't get it to fail. Seems like it might be related to other tests interacting with these. Expanding the testing I'm doing to see if I can track down what specifically is require to get it to fail. I'd say we can move this to WIP

@casperklein casperklein marked this pull request as draft August 22, 2021 11:33
@casperklein casperklein changed the title setup.sh: docker_container first, then fall back to docker_image WIP setup.sh: docker_container first, then fall back to docker_image Aug 22, 2021
+ test changes to support
+ test change to wait for smtp port to fix flakey tests since #2104
@NorseGaud NorseGaud changed the title WIP setup.sh: docker_container first, then fall back to docker_image setup.sh: docker_container first, then fall back to docker_image Aug 29, 2021
@NorseGaud NorseGaud marked this pull request as ready for review August 29, 2021 00:49
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

Very nice additions!
I think this PRs paves the way for bringing some of the logic of setup.sh in the container itself to make setup.sh independent of the mail server version again. I will have a look after this PR is merged :D

Comment thread .github/workflows/test_merge_requests.yml
@NorseGaud
Copy link
Copy Markdown
Member Author

NorseGaud commented Aug 29, 2021

@georglauterbach I see we have a milestone for 10.2.0. Is it ok if I put it in there?

LGTM 👍🏼

Very nice additions!
I think this PRs paves the way for bringing some of the logic of setup.sh in the container itself to make setup.sh independent of the mail server version again. I will have a look after this PR is merged :D

Thanks!

casperklein
casperklein previously approved these changes Aug 29, 2021
@casperklein
Copy link
Copy Markdown
Member

The tests seem not to finish 😕

@NorseGaud
Copy link
Copy Markdown
Member Author

The tests seem not to finish 😕

Sorry, can you clarify what you're seeing?

@casperklein
Copy link
Copy Markdown
Member

The build-and-test action took incredible long (1h 28m 58s), but have finished successful 👍

@NorseGaud
Copy link
Copy Markdown
Member Author

The build-and-test action took incredible long (1h 28m 58s), but have finished successful 👍

Got it -- I see now.

First that that's happened. I'll add some debug output to the tests and see if we can reproduce it.

@NorseGaud
Copy link
Copy Markdown
Member Author

Hmm, it didn't happen before the commit you made. I wonder how much of this is the PR and not just a random/rare situation.

@NorseGaud
Copy link
Copy Markdown
Member Author

Can't reproduce it... Weird...

@casperklein
Copy link
Copy Markdown
Member

Glitch in the matrix or our CI was just busy with other stuff 😉

@NorseGaud
Copy link
Copy Markdown
Member Author

Glitch in the matrix or our CI was just busy with other stuff 😉

matrix

@georglauterbach
Copy link
Copy Markdown
Member

@NorseGaud can this be rebased and merged?

@NorseGaud
Copy link
Copy Markdown
Member Author

@NorseGaud can this be rebased and merged?

Yep, it's ready. Are we doing 10.2.0?

@georglauterbach
Copy link
Copy Markdown
Member

I drafted v10.2.0, but the milestone has one or two PRs I'd like to see merged :) I will merge this and test :edge until v10.2.0 is released.

@georglauterbach georglauterbach enabled auto-merge (squash) September 6, 2021 10:12
@georglauterbach georglauterbach merged commit 0da66cc into docker-mailserver:master Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci area/scripts area/tests kind/improvement Improve an existing feature, configuration file or the documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants