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 CI verify stage #166

Merged
merged 7 commits into from
Nov 6, 2023
Merged

Fix CI verify stage #166

merged 7 commits into from
Nov 6, 2023

Conversation

Niicck
Copy link
Contributor

@Niicck Niicck commented Nov 3, 2023

All the molecule tests now pass locally for me. Let's see how it works in CI.

I cleaned up all the lingering bugs with our molecule tests, plus some new bugs that only showed up in the last couple of months.

Resolves: #154

Here's what I did:

@Niicck Niicck marked this pull request as draft November 3, 2023 18:01
@Niicck Niicck marked this pull request as ready for review November 4, 2023 04:36
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this work @Niicck, much appreciated!

All looks good to me, let's see whether you can figure out the dokku package install as well.

By the way, it's not quite clear to me why CI doesn't run on the branch on your fork but it should be possible to get it to run there (then you don't need to wait for me to click the "approve" button on the workflows).

for line in output:
match = re.match("Proxy port map:(?P<mapping>.+)", line.strip())
if match:
mappings = match.group("mapping").strip().split(" ")
Copy link
Member

@ltalirz ltalirz Nov 4, 2023

Choose a reason for hiding this comment

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

this is not your bug but it seems to me, mappings here is never set if no match is found.
could you fix this if you don't mind?

Easiest is just to initialize it to an empty list somewhere further up, then you can also remove the if/else in the non-legacy part

Copy link
Contributor Author

@Niicck Niicck Nov 4, 2023

Choose a reason for hiding this comment

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

@Niicck
Copy link
Contributor Author

Niicck commented Nov 4, 2023

I think you can configure the repo to allow non-maintainers to run workflows: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#allowing-select-actions-and-reusable-workflows-to-run

But I don't think it'll be a blocker. I figured out how to run molecule locally with different distros. So I should be able to identify the rest of the issues without needing to run them in the actual CI pipeline

@Niicck
Copy link
Contributor Author

Niicck commented Nov 4, 2023

Alright, let's give it another run. I think all the distros should be working.

@ltalirz ltalirz merged commit 437bb94 into dokku:master Nov 6, 2023
7 checks passed
@ltalirz
Copy link
Member

ltalirz commented Nov 6, 2023

I think you can configure the repo to allow non-maintainers to run workflows: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#allowing-select-actions-and-reusable-workflows-to-run

I believe this is about CI running on the upstream repository in the pull request.
You should be able to run the workflows on your fork independently of any settings on the upstream repo.

In any case, the setting is currently
image
i.e. next time you will no longer be affected.

Thanks a lot for the contribution, much appreciated!

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.

CI: verify stage fails
2 participants