Add force with lease support #124
Conversation
05c9b06 to
85e4b8d
Compare
|
@ZPascal Sorry for the delay, Sure I'll take a look |
d48f8dc to
5229503
Compare
|
Is @jackton1 the PR from your side ready to merge? |
| required: false | ||
| force_with_lease: | ||
| description: 'Determines if force-with-lease push is used' | ||
| required: false |
There was a problem hiding this comment.
Can you provide a default here
| required: false | |
| required: false | |
| default: false |
There was a problem hiding this comment.
Sure, but then it's not consistent with the rest.
| set -e | ||
|
|
||
| INPUT_FORCE=${INPUT_FORCE:-false} | ||
| INPUT_FORCE_WITH_LEASE=${INPUT_FORCE_WITH_LEASE:-false} |
There was a problem hiding this comment.
We can reduce redundant default declarations and provide them in the actions.yml
|
|
||
| git push "${remote_repo}" HEAD:${INPUT_BRANCH} --follow-tags $_FORCE_OPTION $_TAGS; | ||
| if ${INPUT_FORCE_WITH_LEASE}; then | ||
| git push --follow-tags $_FORCE_OPTION $_TAGS; |
There was a problem hiding this comment.
Do we need to use a different set of options and arguments here? It seems like overkill to me.
There was a problem hiding this comment.
Unfortunately, yes. Otherwise, it's not working. The combination of the checkout configuration and the corresponding light and different push command solves the problems.
There was a problem hiding this comment.
Can you provide some more information like logs it might be helpful to know and document the subtleties of using either option.
There was a problem hiding this comment.
I just looked in my records and could not find any more logs. I will repeat the test tonight and provide the corresponding logs.
There was a problem hiding this comment.
Still curious as to why we need to couple the behaviour with the checkout action as opposed to using the specified branch and also pushing without the remote_repo which includes the user-defined token ??
There was a problem hiding this comment.
@jackton1 I fired through a corresponding test setup and ran the two actions with two different versions each.
Version 1 ⇾ Current setup of the push command with force-with-lease support.
Version 2 ⇾ Modified version with rebuilt push command.
You can clearly see in version 1 that the command is not executed and rejected.
Push to branch refs/pull/11/merge
Pushing to https://github.com/ZPascal/grafana_dashboard_templater.git
POST git-receive-pack (8[9](https://github.com/ZPascal/grafana_dashboard_templater/runs/6923390058?check_suite_focus=true#step:4:10)9 bytes)
To https://github.com/ZPascal/grafana_dashboard_templater.git
! [remote rejected] HEAD -> refs/pull/11/merge (deny updating a hidden ref)
error: failed to push some refs to 'https://github.com/ZPascal/grafana_dashboard_templater.git'
Error: Invalid exit code: 1
at ChildProcess.<anonymous> (/home/runner/work/_actions/ad-m/github-push-action/cfc[10](https://github.com/ZPascal/grafana_dashboard_templater/runs/6923390058?check_suite_focus=true#step:4:11)a6161cb45fdaa7cb39dfb870e1ac7c32d3d/start.js:29:21)
at ChildProcess.emit (events.js:3[14](https://github.com/ZPascal/grafana_dashboard_templater/runs/6923390058?check_suite_focus=true#step:4:15):20)
at maybeClose (internal/child_process.js:1022:[16](https://github.com/ZPascal/grafana_dashboard_templater/runs/6923390058?check_suite_focus=true#step:4:17))
at Process.ChildProcess._handle.onexit (internal/child_process.js:287:5) {
code: 1
}
In the modified version 2 everything works as expected.
If you have further questions or want to see log messages, please let me know.
There was a problem hiding this comment.
Hi @jackton1, do you have any further questions?
There was a problem hiding this comment.
Nope looks good. Thanks for the explanation.
There was a problem hiding this comment.
@jackton1 Could you please approve it or should I modify more code lines?
start.sh
Outdated
| git config --local --add safe.directory ${INPUT_DIRECTORY} | ||
|
|
||
| git push "${remote_repo}" HEAD:${INPUT_BRANCH} --follow-tags $_FORCE_OPTION $_TAGS; | ||
| #if ${INPUT_FORCE_WITH_LEASE}; then |
There was a problem hiding this comment.
Do we need the commented lines ?
There was a problem hiding this comment.
I'll uncomment the lines/ revert the tested modifications and squash the changes together.
81c3a1e to
c894fcf
Compare
* Update the documentation
d624b04 to
e5f70b0
Compare
Add
--force-with-leasefunctionality from the following PR on a smoother way.TODO's:
--forceparameter Test action--force-with-leaseparameter Test action