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

add proxy plugin workflow #27

Merged
merged 4 commits into from
Oct 16, 2024
Merged

add proxy plugin workflow #27

merged 4 commits into from
Oct 16, 2024

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Jan 5, 2024

@evgeni evgeni force-pushed the proxy_plugin branch 3 times, most recently from 786b9a9 to 9188c23 Compare January 5, 2024 13:37
@evgeni evgeni marked this pull request as ready for review January 5, 2024 13:39
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I'm wondering if this makes sense at all. Unlike Foreman plugins, Smart Proxy plugins are plain gems that can be installed (by depending on smart_proxy in Gemfile). So you can just use our existing gem-test workflow. It respects required_ruby_version in the gemspec and tests on all of them.

You can argue that we somehow want to enforce that it includes all the Ruby versions we care about, but is this the way to go?

@evgeni
Copy link
Member Author

evgeni commented Jan 5, 2024

Well, I like opinionated solutions.
Forcing the matrix to match the one from the main proxy repo is nice.
Also allowing testing with different proxy versions (even if that's not implemented today, we could do it similarly how we test different puppets by looking at ENV[PUPPET_VERSION])

.github/workflows/smart_proxy_plugin.yml Outdated Show resolved Hide resolved
.github/workflows/smart_proxy_plugin.yml Outdated Show resolved Hide resolved
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Do we want to add a "Test suite" check at the end that we can mark as a required check?

@evgeni
Copy link
Member Author

evgeni commented Aug 14, 2024

Do we want to add a "Test suite" check at the end that we can mark as a required check?

There already is

# A dummy job that you can mark as a required check instead of each individual test
test-suite:

Can't this be used as the "required" one?

@ekohl
Copy link
Member

ekohl commented Aug 16, 2024

Oh yes, I suppose so.

Can we create a workflow somewhere to test this out? Perhaps in this repo itself, providing CI for CI?

@evgeni
Copy link
Member Author

evgeni commented Aug 16, 2024

@evgeni
Copy link
Member Author

evgeni commented Aug 16, 2024

For "in this repo" we'd need to teach test-gem to check out arbitrary repos -- should we?

@ekohl
Copy link
Member

ekohl commented Oct 11, 2024

For "in this repo" we'd need to teach test-gem to check out arbitrary repos -- should we?

I wouldn't mind. I suppose an alternative is to introduce some dummy gemspec, but not sure if that's any better.

@evgeni evgeni force-pushed the proxy_plugin branch 3 times, most recently from 7735e55 to 1c2fb02 Compare October 14, 2024 08:21
@evgeni
Copy link
Member Author

evgeni commented Oct 14, 2024

@ekohl implemented self-testing :)

@evgeni
Copy link
Member Author

evgeni commented Oct 14, 2024

The [TMP] commits gotta be dropped before merging :)

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Overall ACK but needs an update now that the required PRs have been merged.

.github/workflows/test-gem.yml Outdated Show resolved Hide resolved
.github/workflows/smart_proxy_plugin_workflow_test.yml Outdated Show resolved Hide resolved
@evgeni
Copy link
Member Author

evgeni commented Oct 15, 2024

@ekohl this still contains two TMP commits that need dropping before merging, tests are green right now. If you ack, I'll drop the two and merge?

@evgeni evgeni merged commit f77f064 into v0 Oct 16, 2024
1 check passed
@evgeni evgeni deleted the proxy_plugin branch October 16, 2024 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants