Conversation
martencassel
force-pushed
the
add_foreman_ci_action
branch
10 times, most recently
from
52fcf2f to
6cad2e3
Compare
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
Is there a specific benefit to switch to minitest over test-unit?
martencassel
force-pushed
the
add_foreman_ci_action
branch
2 times, most recently
from
0c40591 to
3c2fa6b
Compare
martencassel
force-pushed
the
add_foreman_ci_action
branch
4 times, most recently
from
80b67cd to
571931c
Compare
martencassel
changed the title
martencassel
changed the title
martencassel
changed the title
martencassel
force-pushed
the
add_foreman_ci_action
branch
5 times, most recently
from
423a0f8 to
ffbc887
Compare
|
Thanks for reducing the scope. Some small points but otherwise it looks good |
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
Looks like I only left a comment before without submitting the review itself.
martencassel
force-pushed
the
add_foreman_ci_action
branch
from
ffbc887 to
130735f
Compare
|
Hi, i think were ready for your merge action @ekohl when you have the time 😊 |
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
I see many files no longer have a newline (note the last character in the diff). Could you ensure these are present?
martencassel
force-pushed
the
add_foreman_ci_action
branch
5 times, most recently
from
27005f9 to
3afbf8e
Compare
martencassel
force-pushed
the
add_foreman_ci_action
branch
from
3afbf8e to
d7f8a44
Compare
|
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 rkerberos/rkerberos#27. You are at least missing the change to |
|
Changes to 8 files, 7 files to many right ? :-) |
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
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
| ] | ||
| } No newline at end of file | ||
| } | ||
|
|
| apt-get install adcli | ||
| adcli info --domain=EXAMPLE.COM --domain-controller=dc.example.com No newline at end of file | ||
| adcli info --domain=EXAMPLE.COM --domain-controller=dc.example.com | ||
|
|
| on: | ||
| - pull_request | ||
| - push | ||
| on: pull_request |
There was a problem hiding this comment.
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:
| on: pull_request | |
| on: | |
| pull_request: | |
| push: | |
| branches: | |
| - master |
There was a problem hiding this comment.
Thanks for your patience ;-)
There was a problem hiding this comment.
Sorry misunderstood you, its about discrete units of changes, not how many files your chaning per PR right. Thanks
There was a problem hiding this comment.
Yes, indeed. Though I think it ok to have multiple commits in the same PR.
|
We can close and replace with other prs @ekohl |
|
We can fix rubocop changes and then gem spec and then ci pr then i will rebase the remaining ? |
2 participants
Use the default theforeman/action action for smart proxy plugins.