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

Documentation should be updated to not to say all arguments are optional #414

Closed
mattwelke opened this issue Apr 8, 2021 · 5 comments · Fixed by #467
Closed

Documentation should be updated to not to say all arguments are optional #414

mattwelke opened this issue Apr 8, 2021 · 5 comments · Fixed by #467

Comments

@mattwelke
Copy link

I added this action to a repo I maintain (https://github.com/schema-inspector/schema-inspector), using the following:

name: 'Close stale issues and PRs'
on:
  schedule:
    - cron: '54 2 * * *'

jobs:
  stale:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/stale@v3

I chose not to set any arguments because the readme for the action states:

Every argument is optional.

I figured this meant that there would be a default value for the message to use when it detects a stale issue etc and does its thing, and that I could just add an argument later if I wanted to override this default message.

Instead, when it ran overnight, I saw this in the action logs:

Run actions/stale@v3
[#97] Found this pull request last updated 2021-04-07T01:47:53Z
[#97] The option "onlyLabels" was not specified. Continuing the process for this pull request
[#97] Days before pull request stale: 60
[#97] Skipping pull request due to empty stale message
[#71] Found this issue last updated 2021-02-03T23:09:10Z
[#71] The option "onlyLabels" was not specified. Continuing the process for this issue
[#71] Days before issue stale: 60
[#71] Skipping issue due to empty stale message
[#69] Found this issue last updated 2021-04-05T19:36:35Z
[#69] The option "onlyLabels" was not specified. Continuing the process for this issue
[#69] Days before issue stale: 60
[#69] Skipping issue due to empty stale message
[#66] Found this issue last updated 2017-09-25T19:14:33Z
[#66] The option "onlyLabels" was not specified. Continuing the process for this issue
[#66] Days before issue stale: 60
[#66] Skipping issue due to empty stale message
[#62] Found this issue last updated 2017-06-20T18:07:24Z
[#62] The option "onlyLabels" was not specified. Continuing the process for this issue
[#62] Days before issue stale: 60
[#62] Skipping issue due to empty stale message
[#56] Found this issue last updated 2016-11-27T10:08:17Z
[#56] The option "onlyLabels" was not specified. Continuing the process for this issue
[#56] Days before issue stale: 60
[#56] Skipping issue due to empty stale message
[#45] Found this issue last updated 2016-10-28T09:30:31Z
[#45] The option "onlyLabels" was not specified. Continuing the process for this issue
[#45] Days before issue stale: 60
[#45] Skipping issue due to empty stale message
[#40] Found this pull request last updated 2021-04-07T01:11:06Z
[#40] The option "onlyLabels" was not specified. Continuing the process for this pull request
[#40] Days before pull request stale: 60
[#40] Skipping pull request due to empty stale message
---
Statistics
Processed issues/PRs: 8
Operations performed: 4
Fetched issues: 2
---
No more issues found to process. Exiting.

It looks like it's detecting an issue as old (like the one last updated 2016-11-27T10:08:17Z) but choosing not to do anything for that issue.

I checked the readme for this action again and found that the arguments that had to do with the message to be used didn't have defaults, so I think they actually aren't optional. But it isn't clear, since the top of the readme currently says all arguments are optional.

I think the readme should be updated to clearly show which are optional and which aren't.

@C0ZEN
Copy link
Contributor

C0ZEN commented Apr 13, 2021

It is optional from the POV of the action options.
It is not mandatory to specify an option to make the stale action works so on the paper it's simple to use.

IMO it's old and was not challenged since quite some time between the fixes and the new features - or they is a hidden reason I am not aware of.
You are right to point out that it should work out of the box with a "recommended configuration".
I think it's ok to alter some default values because I do not see a lot of people using it without overriding the configuration but it should be clear that a change in this direction should be tackle safely when doing the upgrade so having a PR to handle this change through a breaking change (major tag) would be the best option to achieve that IMO.

@mattwelke
Copy link
Author

Oh I see what you're saying. It has many features, which is why nothing works out of the box until you opt in.

I think that's fine, and we just need some more docs for use cases. Like sample YAML if someone just wants stale issues to be labelled and closed.

@C0ZEN
Copy link
Contributor

C0ZEN commented Apr 28, 2021

IMO blocked by #399

@C0ZEN
Copy link
Contributor

C0ZEN commented May 23, 2021

@luketomlinson @hross once #456 landed to improve the documentation and #457 which create a breaking change, it would be a good time to improve the default options to have only one breaking change. WDYT?

@C0ZEN
Copy link
Contributor

C0ZEN commented May 25, 2021

I can provide a small PR to handle this

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 a pull request may close this issue.

2 participants