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.

Sign up for GitHub

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

Jump to bottom

Update CI #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update CI #38

wants to merge 1 commit into from
+38 −51

Conversation

martencassel
Copy link
Contributor

@martencassel martencassel commented Jan 2, 2025 •
edited
Loading

Use the default theforeman/action action for smart proxy plugins.

@martencassel martencassel force-pushed the add_foreman_ci_action branch 10 times, most recently from 52fcf2f to 6cad2e3 Compare January 2, 2025 19:23
ekohl
ekohl reviewed Jan 2, 2025
View reviewed changes
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.

Is there a specific benefit to switch to minitest over test-unit?

@martencassel martencassel force-pushed the add_foreman_ci_action branch 2 times, most recently from 0c40591 to 3c2fa6b Compare January 2, 2025 19:28
ekohl
ekohl reviewed Jan 2, 2025
View reviewed changes
@martencassel martencassel force-pushed the add_foreman_ci_action branch 4 times, most recently from 80b67cd to 571931c Compare January 2, 2025 19:43
@martencassel martencassel changed the title WIP: Add foreman proxy CI action Add foreman proxy CI action Jan 2, 2025
@martencassel martencassel changed the title Add foreman proxy CI action Update CI action Jan 2, 2025
@martencassel martencassel changed the title Update CI action Update CI - Use theforeman/action Smart Proxy plugin test Jan 2, 2025
@martencassel martencassel changed the title Update CI - Use theforeman/action Smart Proxy plugin test Update CI Jan 2, 2025
@martencassel martencassel force-pushed the add_foreman_ci_action branch 5 times, most recently from 423a0f8 to ffbc887 Compare January 2, 2025 20:03
@ekohl
Copy link
Member

ekohl commented Jan 2, 2025

Thanks for reducing the scope. Some small points but otherwise it looks good

ekohl
ekohl reviewed Jan 2, 2025
View reviewed changes
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.

Looks like I only left a comment before without submitting the review itself.

@martencassel martencassel force-pushed the add_foreman_ci_action branch from ffbc887 to 130735f Compare January 3, 2025 08:04
@martencassel
Copy link
Contributor Author

Hi, i think were ready for your merge action @ekohl when you have the time 😊

ekohl
ekohl requested changes Jan 17, 2025
View reviewed changes
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 see many files no longer have a newline (note the last character in the diff). Could you ensure these are present?

@martencassel martencassel force-pushed the add_foreman_ci_action branch 5 times, most recently from 27005f9 to 3afbf8e Compare January 17, 2025 21:49
@martencassel martencassel force-pushed the add_foreman_ci_action branch from 3afbf8e to d7f8a44 Compare January 17, 2025 22:08
@martencassel martencassel requested a review from ekohl January 17, 2025 22:10
@ekohl
Copy link
Member

ekohl commented Feb 18, 2025

I think there are many changes in this PR that aren't strictly needed for the conversion, so I tried to locally replicate it but ran into domcleal/rkerberos#27.

You are at least missing the change to Gemfile which is shown in the last line on https://github.com/theforeman/actions?tab=readme-ov-file#smart-proxy-plugin-test.

@martencassel
Copy link
Contributor Author

martencassel commented Feb 18, 2025 •
edited
Loading

Changes to 8 files, 7 files to many right ? :-)

ekohl
ekohl requested changes Feb 18, 2025
View reviewed changes
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 have a preference of separating things more, like a separate commit just for whitespace changes for example. For example, ekohl@d9729aa first to clean up some old stuff, then ekohl@e3812e8 just to update the CI. Then I ran into the other things you noticed, which is the stricter gemspec. I'd probably add those as separate commits just for visibility.

The above is my personal style because I've learned it's useful in larger projects that run for years where you often use git blame to understand what happened and why. Then I also really like https://cbea.ms/git-commit/ to properly describe the changes. Hint: if you want to use and in your summary, you may want to split things up.

But in the end it doesn't matter that much. This is a fairly small plugin so feel free to ignore it.

Please do respect SMART_PROXY_BRANCH in Gemfile. See ekohl@e3812e8

.vscode/launch.json
}

Copy link
Member

Choose a reason for hiding this comment

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

Nit: here's an empty line.

test/setup_ad.md
adcli info --domain=EXAMPLE.COM --domain-controller=dc.example.com

Copy link
Member

Choose a reason for hiding this comment

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

And here too

.github/workflows/ci.yml
on:
- pull_request
- push
on: pull_request
Copy link
Member

Choose a reason for hiding this comment

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

In the examples we simplified it to just pull_request, but in practice there is a benefit to using push: it builds a common cache. To prevent duplicate runs you can limit it to just running on master:

Suggested change
on: pull_request
on:
pull_request:
push:
branches:
- master

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your patience ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry misunderstood you, its about discrete units of changes, not how many files your chaning per PR right. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Yes, indeed. Though I think it ok to have multiple commits in the same PR.

@martencassel
Copy link
Contributor Author

We can close and replace with other prs @ekohl

@martencassel
Copy link
Contributor Author

martencassel commented Mar 4, 2025 •
edited
Loading

We can fix rubocop changes and then gem spec and then ci pr then i will rebase the remaining ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl requested changes

Assignees
No one assigned
Labels
Waiting on contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@martencassel @ekohl