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

test: add test for networking setup in act #1375

Merged
merged 4 commits into from
Oct 12, 2022
Merged

Conversation

KnisterPeter
Copy link
Member

@KnisterPeter KnisterPeter commented Oct 7, 2022

This test makes sure that the hostname inside of act is resolvable.
This will prevent regressions in the network setup (see #1351, #1022)

Act need to be started with docker network mode host by default. This was changed recently, leading to networking changes. That do break the previous (and expected) behavior.

This test makes sure that the hostname inside of act is resolvable.
@KnisterPeter KnisterPeter self-assigned this Oct 7, 2022
@KnisterPeter KnisterPeter requested a review from a team as a code owner October 7, 2022 08:43
@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2022

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 2 0 0.02s
✅ REPOSITORY gitleaks yes no 2.44s
✅ REPOSITORY git_diff yes no 0.0s
✅ REPOSITORY secretlint yes no 1.01s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@mergify
Copy link
Contributor

mergify bot commented Oct 7, 2022

@KnisterPeter this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Oct 7, 2022
When merging parsed container options without options being
set in a job, the default docker options are returned and
will override the expected defaults by act (e.g. network mode).

This is a first attempt to mitigate this behavior and only
merge settings if something was requested on a job.
@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 7, 2022
@mergify
Copy link
Contributor

mergify bot commented Oct 7, 2022

@KnisterPeter this pull request has failed checks 🛠

@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Merging #1375 (b60da5a) into master (4f8da0a) will increase coverage by 6.51%.
The diff coverage is 75.15%.

@@            Coverage Diff             @@
##           master    #1375      +/-   ##
==========================================
+ Coverage   57.50%   64.02%   +6.51%     
==========================================
  Files          32       41       +9     
  Lines        4594     6405    +1811     
==========================================
+ Hits         2642     4101    +1459     
- Misses       1729     2009     +280     
- Partials      223      295      +72     
Impacted Files Coverage Δ
pkg/model/action.go 0.00% <0.00%> (ø)
pkg/model/step_result.go 0.00% <ø> (ø)
pkg/container/docker_run.go 12.82% <11.63%> (+7.27%) ⬆️
pkg/model/planner.go 48.82% <25.00%> (-1.60%) ⬇️
pkg/model/workflow.go 50.97% <32.14%> (+0.05%) ⬆️
pkg/container/docker_pull.go 33.33% <33.33%> (ø)
pkg/container/file_collector.go 45.87% <45.87%> (ø)
pkg/common/git/git.go 50.00% <47.91%> (ø)
pkg/container/docker_auth.go 47.61% <50.00%> (+2.61%) ⬆️
pkg/exprparser/interpreter.go 73.37% <53.48%> (-0.02%) ⬇️
... and 31 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mergify mergify bot removed the needs-work Extra attention is needed label Oct 7, 2022
@KnisterPeter
Copy link
Member Author

@alex-savchuk Please have a look as well next to the maintainers 🙏

@KnisterPeter KnisterPeter added the kind/bug Something isn't working label Oct 7, 2022
@alex-savchuk
Copy link
Contributor

@KnisterPeter Hello.
Looking at this issue right now.

@alex-savchuk
Copy link
Contributor

Looks good from my point of view.

Main change, as i understand, is that we do not merge nothing if options are empty. With my solution we did.

But i see that test does not catch the problem but only prints data for manual checking.
Maybe we somehow pass hostname of host system as input and verify that it equals to current container hostname?
But maybe current test is enough.

Thank you for catching this problem and providing solution!
I'm traveling right now so wouldn't be able to fix that at least until Wednesday :(

@KnisterPeter
Copy link
Member Author

Main change, as i understand, is that we do not merge nothing if options are empty. With my solution we did.

Exactly. I'm still unsure if that is good enough, but it should restore basic functionality.

But i see that test does not catch the problem but only prints data for manual checking. Maybe we somehow pass hostname of host system as input and verify that it equals to current container hostname? But maybe current test is enough.

The test host $(hostname -f) does fail with your change, but succeeds with the expected state.
At least on linux and GitHub it does, so it should be sufficient I think.

Thank you for catching this problem and providing solution! I'm traveling right now so wouldn't be able to fix that at least until Wednesday :(

No problem, I did not see this issue while I've reviewed your code. This happens all the time as our tests are not yet good enough. A fix is already included in this PR.

@mergify mergify bot merged commit ff5e289 into master Oct 12, 2022
@mergify mergify bot deleted the add-networking-test branch October 12, 2022 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants