Skip to content

Add minimumReleaseAge#2065

Merged
zbrydon merged 14 commits into
mainfrom
add-minimumReleaseAge
Sep 23, 2025
Merged

Add minimumReleaseAge#2065
zbrydon merged 14 commits into
mainfrom
add-minimumReleaseAge

Conversation

@zbrydon

@zbrydon zbrydon commented Sep 18, 2025

Copy link
Copy Markdown
Contributor

@zbrydon
zbrydon requested a review from a team as a code owner September 18, 2025 04:00
@changeset-bot

changeset-bot Bot commented Sep 18, 2025

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 11e3c21

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@zbrydon
zbrydon marked this pull request as draft September 18, 2025 22:35

@tadhglewis tadhglewis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm - gotta stop those zero days

}

const newContents = `${pnpmWorkspaceContents}
minimumReleaseAge: 1440

@tadhglewis tadhglewis Sep 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about 3 days? Potentially overly cautious however renovates would take a few days to update a dependency anyway, so we should align local pnpm install with the renovate schedule?

Ref: https://github.com/seek-oss/rynovate/blob/714d3dc0de53be06c5228bed5e550ae65cab3ab7/default.json#L321

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah open to suggestions, picked a day because that's what was in the pnpm changelog 😆

Mhmmm didn't think about that, kinda like aligning them 🤔

@zbrydon
zbrydon marked this pull request as ready for review September 19, 2025 03:37

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Went with this approach because reviving this seemed like a substantially larger undertaking.

I would like to see it happen, but given the current frequency of incidents I thought the value of getting this in sooner rather than later outweighs having a nicer way to manage config now. 🫠

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have an immediate need for overloads? If not, I wonder if we leave this out (or perhaps undocumented so as to not encourage uptake) until we implement skuba.config.ts properly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Think yes, only if we end up not excluding packages that we want to publish and test quickly 🤔

I wound it back to just @seek/* but am now reconsidering after looking at all the exclusions listed here.

Feel like it might be good to have them the same so we don't end up with renovates hanging around unable to install...

Comment thread template/base/_pnpm-workspace.yaml Outdated
Comment thread template/base/_pnpm-workspace.yaml Outdated
Comment on lines +5 to +6
- '*skuba*'
- '*seek*'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it concerning to exempt package patterns that we don't control (i.e. other than @seek/*)?

I'm thinking: not great, but probably not a dealbreaker. skuba isn't going to appear in package names organically, so it would require a targeted attack and two steps to exploit (compromise a package left-pad to add a benign dependency on a bad-skuba@^1.0.0 package then wait for 3 days for left-pad to be bumped, publish evil-skuba@1.0.1 with badness included then wait for the next lock file maintenance). seek may be more likely but again would likely require a targeted attack.

Not sure I like the alternative of tediously listing out the exclusions, unless we keep it to a pretty small subset (e.g. it's probably fine if seek-koala is not updated immediately).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah thinking through it more, the motivation was to keep the exclusion list short while not getting in the way of development. But if we keep the ability to overload then that provides an escape hatch if required, and makes the default less greedy. 🤔

Comment thread src/cli/configure/processing/configFile.ts Outdated
Comment thread src/cli/configure/processing/configFile.ts Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have an immediate need for overloads? If not, I wonder if we leave this out (or perhaps undocumented so as to not encourage uptake) until we implement skuba.config.ts properly.

Comment thread .changeset/chubby-tables-hug.md Outdated
Comment thread pnpm-workspace.yaml Outdated
Comment thread template/base/_pnpm-workspace.yaml Outdated
Co-authored-by: Ryan Ling <ryan@outlook.com.au>
@zbrydon
zbrydon merged commit fe2df2f into main Sep 23, 2025
18 checks passed
@zbrydon
zbrydon deleted the add-minimumReleaseAge branch September 23, 2025 03:41
@seek-oss-ci seek-oss-ci mentioned this pull request Sep 23, 2025
72636c added a commit to seek-oss/rynovate that referenced this pull request Sep 23, 2025
72636c added a commit to seek-oss/rynovate that referenced this pull request Sep 28, 2025
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.

3 participants