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

feat: do not fail on already existing rocks, add fail_on_duplicate flag #411

Merged
merged 1 commit into from
May 25, 2024

Conversation

vhyrro
Copy link
Contributor

@vhyrro vhyrro commented May 23, 2024

This pull request adds a fail_on_duplicate flag to the github action (set to false by default). This will prevent the workflow from failing if the rock already exists on the server.

This should greatly simplify the workflows people use for releasing their plugin, as now the workflow should be able to run every time a commit is made to a repository.

Fixes #377.

Things to do:

  • Add mention of the option in the README

@vhyrro vhyrro force-pushed the push-nrukxqoxtwoz branch 2 times, most recently from 88fca8a to 9303022 Compare May 23, 2024 14:56
@vhyrro vhyrro marked this pull request as ready for review May 23, 2024 14:56
@vhyrro vhyrro requested a review from mrcjkb May 23, 2024 14:57
@vhyrro
Copy link
Contributor Author

vhyrro commented May 23, 2024

Pull request is ready now. Let me know if I missed something critical or obvious :)

@vhyrro
Copy link
Contributor Author

vhyrro commented May 23, 2024

https://github.com/nvim-neorg/neorg/actions/runs/9211062730/job/25339613079 hmm, weird? 🤔

Very difficult to assess the source of the error here

@mrcjkb
Copy link
Member

mrcjkb commented May 23, 2024

https://github.com/nvim-neorg/neorg/actions/runs/9211062730/job/25339613079 hmm, weird? 🤔

Very difficult to assess the source of the error here

Error: Revision 8.5.0-1 already exists on the server. See '/nix/store/zcncmwl2ia3pxx1xill67w4d6cjwwzdi-luarocks-3.9.2/bin/.luarocks-wrapped help upload'.

lua/luarocks-tag-release.lua Outdated Show resolved Hide resolved
lua/luarocks-tag-release.lua Outdated Show resolved Hide resolved
@vhyrro
Copy link
Contributor Author

vhyrro commented May 23, 2024

https://github.com/nvim-neorg/neorg/actions/runs/9211062730/job/25339613079 hmm, weird? 🤔
Very difficult to assess the source of the error here

Error: Revision 8.5.0-1 already exists on the server. See '/nix/store/zcncmwl2ia3pxx1xill67w4d6cjwwzdi-luarocks-3.9.2/bin/.luarocks-wrapped help upload'.

Hah that much I could see, I meant the source of why the fallback mechanism doesn't seem to be triggering properly at all. I can't see the search command being executed in the verbose logs (even though it should).

@mrcjkb mrcjkb enabled auto-merge (squash) May 24, 2024 05:31
@vhyrro vhyrro force-pushed the push-nrukxqoxtwoz branch from 9303022 to 4eb707e Compare May 25, 2024 20:05
Comment on lines 96 to 108
local stdout, _ = OS.execute(cmd, function(message)
if message:find("already exists on the server") and not args.fail_on_duplicate then
print(
string.format(
'%s already exists with version %s on the remote. Doing nothing (`fail_on_duplicate` is false).'
)
)
else
error(message)
end
end, args.is_debug)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do let me know if it's more in your style to break this out into a separate function :)

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with this as it is.

@vhyrro vhyrro force-pushed the push-nrukxqoxtwoz branch from 4eb707e to df37006 Compare May 25, 2024 20:09
@vhyrro
Copy link
Contributor Author

vhyrro commented May 25, 2024

Tested over at https://github.com/nvim-neorg/neorg/actions/runs/9237717985/job/25415443099 and it seems to be functional :)

@@ -92,7 +93,19 @@ local function luarocks_tag_release(package_name, package_version, specrev, args
.. ' --api-key $LUAROCKS_API_KEY'
.. luarocks_extra_flags_and_args
print('UPLOAD: ' .. cmd)
local stdout, _ = OS.execute(cmd, error, args.is_debug)
local stdout, _ = OS.execute(cmd, function(message)
if message:find('already exists on the server') and not args.fail_on_duplicate then
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: we should probably add an integration test for this (a workflow that uploads luarocks-tag-release with a hardcoded version), just in case the error message changes after a luarocks update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea, I like that. Shall we do that in a separate PR or in this one?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with creating an issue for it and merging this one.

@mrcjkb mrcjkb merged commit 07c35cf into master May 25, 2024
8 checks passed
@mrcjkb mrcjkb deleted the push-nrukxqoxtwoz branch May 25, 2024 20:33
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.

[Feature] Add option to fail gracefully or force if rock already exists
2 participants