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!: add support for uploading to release drafts. BREAKING CHANGE: remove support for creating a new release. #38

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

Conversation

HarikrishnanBalagopal
Copy link

When we try to get a release by tag it doesn't
return draft releases. In order to get draft
releases we need to list the releases using a
token that has write access on the repo.

Signed-off-by: Harikrishnan Balagopal harikrishmenon@gmail.com

Copy link
Owner

@svenstaro svenstaro left a comment

Choose a reason for hiding this comment

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

Don't commit the changes in dist/ and leave them up to me instead so that we can minimize the changes in this PR.

branding:
icon: archive
color: orange
inputs:
repo_token:
description: 'GitHub token.'
description: "GitHub token."
Copy link
Owner

Choose a reason for hiding this comment

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

Please make the linting-type changes in a separate PR in order to minimize the changes in this one.

@@ -2,11 +2,16 @@
"compilerOptions": {
"target": "es6",
"module": "commonjs",
"moduleResolution": "node",
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, please make linting changes in a separate PR.

Choose a reason for hiding this comment

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

ok. Do you know why the tests are failing?
I added sourceMap flags to the build commands to try and get some better error messages.

From testing on my own repo with ACTIONS_STEP_DEBUG secret set to true, it seems to be working correctly
when the token has the correct permissions.

I think what's happening is the token used in the tests doesn't have enough permissions to list release drafts
or to create new releases.

Copy link
Author

Choose a reason for hiding this comment

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

On a separate note, I don't think this action should create the release if it doesn't exist.

There are already several simple and very flexible actions that create releases:

Creating releases you usually want some sort of changelog computed from the commit messages,
or issues or pull requests. The above actions can do that. In our case the body is a static string at best.

To make things worse the release we create is automatically published as well (it is not a release draft).

I think the scope of this action should be limited to simply uploading assets to existing releases or release drafts.
Leave release creation to other tasks that more designed for that purpose.

Choose a reason for hiding this comment

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

It seems the tests were failing because of a missing await.
It was returning a Promise instead of the result of the promise.

@HarikrishnanBalagopal
Copy link
Author

Don't commit the changes in dist/ and leave them up to me instead so that we can minimize the changes in this PR.

If I do that then this test will fail for sure right?
https://github.com/HarikrishnanBalagopal/upload-release-action/blob/master/.github/workflows/ci.yml#L17
Since it just uses whatever is in dist/ directly without building first.

So it will use the old dist.

@HarikrishnanBalagopal
Copy link
Author

HarikrishnanBalagopal commented Dec 9, 2020

PTAL
I have made a breaking change. Now the action only uses existing releases and release drafts.
The inputs for the action have been simplified to reflect that change.

The test is failing now because there is no existing release or release draft with the tag
ci-test-ubuntu-latest-410155044 for on this repo.

I have tested the code in the following 4 cases using my own repo:

  • tag doesn't exist and release doesn't exist
  • tag exists but no corresponding release
  • tag exists but the release is a draft
  • tag exists and release exists

@HarikrishnanBalagopal HarikrishnanBalagopal changed the title feat: add support for uploading to draft releases feat!: add support for uploading to release drafts. BREAKING CHANGE: remove support for creating a new release. Dec 9, 2020
@HarikrishnanBalagopal
Copy link
Author

HarikrishnanBalagopal commented Dec 9, 2020

I tried changing the tests to match the new behaviour.
It seems during to security issues actions triggered by
pull requests run with less privileges. So the test fails
to create a release.

actions/first-interaction#10

@HarikrishnanBalagopal HarikrishnanBalagopal force-pushed the feat/support-draft branch 7 times, most recently from 5c0676c to c1f7dc8 Compare December 9, 2020 11:11
When we try to get a release by tag, it doesn't
return release drafts. In order to get release
drafts we need to list the releases using a token
that has write access on the repo.

Also added tests for the same.

BREAKING CHANGE: Changed behaviour to not create
a new release. Only upload to existing releases.
Also updated the action inputs to reflect the same.

Signed-off-by: Harikrishnan Balagopal <harikrishmenon@gmail.com>
@svenstaro
Copy link
Owner

I don't want to change the scope of this Action. I think it strikes a good balance between convenience and features. There are plenty other Actions out there if you want separate asset uploading and release creation. Adding a draft mode is fine but please try to minimize the difference in the PR before I do a proper review.

@HarikrishnanBalagopal
Copy link
Author

HarikrishnanBalagopal commented Dec 9, 2020

I got the tests working:
https://github.com/HarikrishnanBalagopal/upload-release-action/actions/runs/410519830

I don't want to change the scope of this Action. I think it strikes a good balance between convenience and features. There are plenty other Actions out there if you want separate asset uploading and release creation. Adding a draft mode is fine but please try to minimize the difference in the PR before I do a proper review.

Sure, but I think the create release stuff should at least be a flag. Default to have the action just fail if the flag is not set.
In most cases if the release is not found that's because of a bug somewhere, either the tag is wrong or a previous action
that was supposed to create the release failed. Having the upload assets actions also create the release just causes more
confusion.

@svenstaro
Copy link
Owner

I got the tests working:
https://github.com/HarikrishnanBalagopal/upload-release-action/actions/runs/410519830

I don't want to change the scope of this Action. I think it strikes a good balance between convenience and features. There are plenty other Actions out there if you want separate asset uploading and release creation. Adding a draft mode is fine but please try to minimize the difference in the PR before I do a proper review.

Sure, but I think the create release stuff should at least be a flag. Default to have the action just fail if the flag is not set.
In most cases if the release is not found that's because of a bug somewhere, either the tag is wrong or a previous action
that was supposed to create the release failed. Having the upload assets actions also create the release just causes more
confusion.

Ok having a flag is fine but I think the default behavior of this action should no be changed as it might surprise current users.

Could you rebase your PR and try to minimize the unnecessary changes such as linting autofixes?

@gerlero gerlero mentioned this pull request May 19, 2023
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.

None yet

2 participants