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

fix 'invalid omit_containers' #1058

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

DavidBruchmann
Copy link
Contributor

Description

this commit tries to fix failing of Run jonaseberle/github-action-setup-ddev@v1 in #1022, by defining containers to omit differently.

if the mentioned run above is working now the PR could be merged, else just dump it, or remove db in omit_containers from .devbox/.ddev/config.yaml.

Note, that the whole failing tests include still another issue, which is not addressed in this commit / PR.

@tomasnorre tomasnorre merged commit 519bae3 into tomasnorre:main Apr 30, 2024
7 of 34 checks passed
@tomasnorre
Copy link
Owner

tomasnorre commented Apr 30, 2024

Thanks. I didn't have problems with this before, and all tests are green in dev-main, but as the docs states your change is right, I have merged it.

@DavidBruchmann
Copy link
Contributor Author

Great that it worked, thanks for the feedback!

@tomasnorre
Copy link
Owner

It's merged, but failed, due to a problem with ddev-github-action atm.
ddev/github-action-setup-ddev#27

@DavidBruchmann
Copy link
Contributor Author

Yes, I know ;-)

Note, that the whole failing tests include still another issue, which is not addressed in this commit / PR.

@DavidBruchmann DavidBruchmann deleted the fixOmitContainers branch April 30, 2024 10:05
@DavidBruchmann
Copy link
Contributor Author

I'm not sure if adding db there is correct, see here: https://github.com/tomasnorre/crawler/actions/runs/8892439954/job/24416494000#step:7:1105

@tomasnorre
Copy link
Owner

I was correct. As the

Yes, I know ;-)

Note, that the whole failing tests include still another issue, which is not addressed in this commit / PR.

I overlooked that comment.

I'm not sure if adding db there is correct, see here: https://github.com/tomasnorre/crawler/actions/runs/8892439954/job/24416494000#step:7:1105

The DB setup should be right, though, but will test when I find the time and energy.

@tomasnorre
Copy link
Owner

There was a regression in the fix I merged of yours.
8d72fea

The reason it worked before with the wrong "dba" in omit_containers, was that it was never meant to be omitted. Therefore it worked before :)

I have adjusted the ddev configs, to avoid confusions like this in the future. Thanks for your contribution, without it, I would have found the problem any time soon.

@DavidBruchmann
Copy link
Contributor Author

Great that you tackled it!
Regrettable only a small contribution from my side ;-)

@tomasnorre
Copy link
Owner

Every contribution has value.

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.

2 participants