Do not update `{major}.json` if an older version was published #483

cleptric · 2023-08-03T10:55:44Z

This should resolve an issue where craft wrongly updates the {major}.json if an older version is released. See getsentry/sentry-release-registry@6db6af4

Update (@Lms24): This PR not only resolves the issue mentioned above but it rewrites the linking cases to take previously existing symlinks into account. This way, we can more safely bump the respective symlinks. Added tests to cover the most important + edge case paths.

src/utils/symlink.ts

asottile-sentry

please add a test demonstrating the behaviour(s)

cleptric · 2023-08-03T14:39:39Z

We currently do not have any tests in place. How would you envision these tests to look like?

asottile-sentry · 2023-08-03T15:02:00Z

there's tests for other targets here -- can follow those as an example https://github.com/getsentry/craft/tree/master/src/targets/__tests__

cleptric · 2023-08-03T15:42:36Z

Could you please provide a bit more specific guidance? How would you like these tests to be structured? Should we mock the symlinks in any way, etc.

asottile-sentry · 2023-08-03T16:06:47Z

I don't particularly care beyond that there are tests for the functionality -- use your judgement to pick what makes sense for you

asottile-sentry · 2023-10-02T13:34:42Z

I don't think this is quite right

consider this scenario:

release 1.0
release 2.0
release 1.0.1

I expect the symlink structure to be:

latest -> 2.0
2 -> 2.0
1 -> 1.0.1

before this patch it would do that I believe but after this patch the 1 symlink will be untouched because it was "older"

I think the actual fix needs to understand the current set of symlinks and make the symlink based on that

tonyo · 2023-10-02T15:50:49Z

@asottile-sentry yep, you're right 👍
We had a chat with @Lms24 , we'll cover that case.

Lms24 · 2023-10-02T15:51:37Z

Talked with @tonyo (writing down for future reference): The missing check is if new.major < old.major, in which case we also need to update the old_major.json file.

asottile-sentry · 2023-10-02T15:52:31Z

I'm not sure that's sufficient either -- the same problem will exist for minor revisions:

1.1.0
1.2.0
1.1.1

+ adjust tests

Lms24 · 2023-10-03T12:33:18Z

Ok, so I rewrote this logic to take existing minor.json and major.json links into account and only rewrite if applicable for them. (latest.json will only be bumped if we're dealing with an actually new, or the first version).
Also added a few tests for scenarios with previously existing symlinks and it seems like things work fine now.

asottile-sentry

new tests look useful! thanks for those!

asottile-sentry · 2023-10-03T13:41:41Z

src/utils/symlink.ts

+    logger.debug(
+      `${
+        newVersionMajorSymlinkedVersion ? 'Updating' : 'Adding'
+      } symlink for "${majorVersionLink}" ${
+        newVersionMajorSymlinkedVersion
+          ? `from version "${semVerToString(newVersionMajorSymlinkedVersion)}" `
+          : ''
+      }to "${newVersion}"`
+    );


there's way too much going on in this line -- if it's just for a debug line I'd just put the raw data in here instead of trying to make it end-user readable. should eliminate all the logic here as well since this seems error prone / more likely to break than the actual logic

I guess we can simplify it a little but while working on this, having some readable debug output was immensly helpful. (Fwiw, what we had before showed the wrong "from version" on many occasions).

I think logger.debug('symlinking', {before, after}) would probably be sufficient

updated it again

asottile-sentry · 2023-10-03T13:44:38Z

src/utils/symlink.ts

+  try {
+    // using lstat instead of exists because broken symlinks return false for exists
+    fs.lstatSync(symlinkPath);
+    const linkedFile = fs.readlinkSync(symlinkPath);
+    return parseVersion(path.basename(linkedFile));
+  } catch {


this try catch is overly broad -- each of these 3 statements could raise errors but I think (?) you only intend to catch errors from the first one? this could hide a programming error especially after future maintenance

good catch!

asottile-sentry · 2023-10-03T13:45:36Z

src/utils/symlink.ts

-  const majorVersionLink = `${parsedNewVersion.major}.json`;
-  forceSymlink(baseVersionName, path.join(packageDir, majorVersionLink));
+  // Read possibly existing symlinks for major and minor versions of the new version
+  const newVersionMajorSymlinkedVersion = getExistingSymlinkedVersion(


this variable name seems overly verbose and also incorrect -- maybe existingMajorSymlink instead ? (same for the other one right below this)

I replaced it with existingLinkedMajorVersion which hopefully clarifies what this variable is holding (i.e. not the symlink but the already parsed version that this link is linking to).

asottile-sentry · 2023-10-03T13:47:03Z

src/utils/__tests__/symlink.test.ts

+  describe('correctly handles already existing symlinks', () => {
+    it('when updating an old major version', async () =>


the describe / its read backwards here -- I think they're supposed to be the other way around so the code "reads"?

I tried to "group" the three tests that cover multiple link calls and hence handle actually existing files/symlinks.
I'll just remove the describe block here to clear up the confusion.

asottile-sentry

Do not update {major}.json if an older version was published

8ee09d0

BYK reviewed Aug 3, 2023

View reviewed changes

src/utils/symlink.ts Outdated Show resolved Hide resolved

BYK reviewed Aug 3, 2023

View reviewed changes

src/utils/symlink.ts Outdated Show resolved Hide resolved

Move log stuff around

bc32cc6

cleptric marked this pull request as ready for review August 3, 2023 12:05

tonyo approved these changes Aug 3, 2023

View reviewed changes

cleptric requested a review from a team August 3, 2023 12:13

asottile-sentry reviewed Aug 3, 2023

View reviewed changes

tonyo added 2 commits October 2, 2023 14:51

add tests

7300fa4

Merge branch 'master' into fix-major-symlinks

836a11f

rewrite symlink updating logic based on existing symlinks

149be13

+ adjust tests

asottile-sentry reviewed Oct 3, 2023

View reviewed changes

Lms24 added 2 commits October 3, 2023 16:18

apply review feedback

94d1ce1

further simplify logger and fix link lookup

01a3661

asottile-sentry approved these changes Oct 3, 2023

View reviewed changes

Lms24 merged commit e391121 into master Oct 3, 2023
8 checks passed

Lms24 deleted the fix-major-symlinks branch October 3, 2023 14:41

mydea mentioned this pull request Nov 15, 2023

version: latest seems to use most recent release instead of latest release matbour/setup-sentry-cli#17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not update `{major}.json` if an older version was published #483

Do not update `{major}.json` if an older version was published #483

cleptric commented Aug 3, 2023 •

edited by Lms24

Loading

asottile-sentry left a comment

cleptric commented Aug 3, 2023

asottile-sentry commented Aug 3, 2023

cleptric commented Aug 3, 2023

asottile-sentry commented Aug 3, 2023

asottile-sentry commented Oct 2, 2023

tonyo commented Oct 2, 2023

Lms24 commented Oct 2, 2023

asottile-sentry commented Oct 2, 2023 •

edited

Loading

Lms24 commented Oct 3, 2023

asottile-sentry left a comment

asottile-sentry Oct 3, 2023

Lms24 Oct 3, 2023

asottile-sentry Oct 3, 2023

Lms24 Oct 3, 2023

asottile-sentry Oct 3, 2023

Lms24 Oct 3, 2023

asottile-sentry Oct 3, 2023

Lms24 Oct 3, 2023

asottile-sentry Oct 3, 2023

Lms24 Oct 3, 2023

asottile-sentry left a comment

		describe('correctly handles already existing symlinks', () => {
		it('when updating an old major version', async () =>

Do not update {major}.json if an older version was published #483

Do not update {major}.json if an older version was published #483

Conversation

cleptric commented Aug 3, 2023 • edited by Lms24 Loading

asottile-sentry left a comment

Choose a reason for hiding this comment

cleptric commented Aug 3, 2023

asottile-sentry commented Aug 3, 2023

cleptric commented Aug 3, 2023

asottile-sentry commented Aug 3, 2023

asottile-sentry commented Oct 2, 2023

tonyo commented Oct 2, 2023

Lms24 commented Oct 2, 2023

asottile-sentry commented Oct 2, 2023 • edited Loading

Lms24 commented Oct 3, 2023

asottile-sentry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asottile-sentry left a comment

Choose a reason for hiding this comment

Do not update `{major}.json` if an older version was published #483

Do not update `{major}.json` if an older version was published #483

cleptric commented Aug 3, 2023 •

edited by Lms24

Loading

asottile-sentry commented Oct 2, 2023 •

edited

Loading