fix(#596) fix issue with semver validation for latest versions #597

1yuv · 2024-01-05T19:43:40Z

Description

[description]

#596

Code review items

Readable: Concise, well named, follows the style guide, documented if necessary.
Documented: Configuration and user documentation on cht-docs
Tested: Unit and/or integration tests where appropriate
Backwards compatible: Works with existing data and configuration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

kennsippell

Can you please add a test to capture this scenario clearly?

kennsippell · 2024-01-06T04:49:33Z

src/fn/upload-app-settings.js

@@ -39,7 +39,7 @@ async function augmentDeclarativeWithNoolsBoilerplate(appSettings) {
  }

  const actualCoreVersion = await getValidApiVersion(appSettings);
-  const addNoolsBoilerplate = semver.lt(actualCoreVersion, '4.2.0-dev');
+  const addNoolsBoilerplate = actualCoreVersion && semver.lt(actualCoreVersion, '4.2.0-dev');


I'm unclear on when actualCoreVersion would be falsy... Why?

It may not be safe to default to nools... for example: a partner using 3rd party library which isn't nools compatible upgrades to a cht-core verison with non-semver version. Now their config is broken when they redeploy...

Even though coerce tries to parse versions correctly, it'd be falsy when a custom version like tag number or branch number is being used on production. What do you suggest in this case to avoid scenarios like you mentioned? One option is to use base_Version which is only available in latest CHT versions and ensure this doesn't get changed. This issue was also caused because the way version was written was changed.

Can you share the specific tags you're having trouble with please? I'm testing with coerce and not clear on what tags are failing. Maybe capture them in the test scenarios if they are special considerations that we need to think through?

This issue fix was needed for regular CHT versions as well like 4.5.0.6922454971, hence I introduced coerce for that fix. actualCoreVersion could still be falsy if production instances are running on versions like this (private). We won't need falsy check after introducing coerce. I don't see recent instances with unusual versioning though. I am not sure what coreVersion would've returned for particular versions like that since api/deploy-info endpoint has changed frequently.

src/lib/get-api-version.js

kennsippell · 2024-01-08T17:43:55Z

test/fn/upload-app-settings.spec.js

@@ -20,6 +20,10 @@ describe('upload-app-settings', () => {

    // case when testing on dev
    { coreVersion: '4.2.0-dev.1682192676689', isDeclarative: true, expectNools: false },
+    // https://github.com/medic/cht-conf/issues/596
+    { coreVersion: '4.5.0.6922454971', isDeclarative: false, expectNools: false},


can you add the case for isDeclarative: true, expectNools: false with this weird build version? isn't that the case that crashed for you?

It was failing with version 4.5.0.6922454971, if we pass weird build version, it'd faill.

If I revert your changes in src, all these tests are still passing... This means that you have not sufficiently captured the scenario you care about in these tests.

That's right, because the fix was on getValidApiVersion() and in this example, we're mocking what getValidApiVersion() returns. Previously it was failing on getValidApiVersion() and failing upload-app-settings. I can add unit test for getApiVersion() method if that makes it more useful.

This isn't a super obvious requirement, so if you want it to not break in the future - it is typically nice to have a test which captures the requirement. Ya - that can be a unit test for getApiVersion or an integration test here (change what is mocked to be deeper).

jkuester

I have nothing to add on top of what Kenn has said! Approving this preemptively so you can merge as soon as Kenn's concerns are resolved!

1yuv · 2024-01-09T00:40:26Z

Hi @kennsippell , I've added few more scenarios for unit test. The original inetnt of this issue was to fix the scenarios where valid CHT core versions like 4.5.0.xxxx were not being parsed properly and an error was being thrown.

Before this fix, if you were running upload-app-settings against any CHT instance with 4.5.0, it'd fail because /api/deployinfo returns version 4.5.0.6922454971 causing actualCoreVersion to be falsy and crash when running semver.lt(). semver.coerce would correctly parse 4.5.0.6922454971 to 4.5.0 and fix the problem.

Even after adding coerce, I added check on if actualCoreVersion is undefined or not to stop it from failing.

const addNoolsBoilerplate = actualCoreVersion && semver.lt(actualCoreVersion, '4.2.0-dev');

However, if you think we should still crash the app and prevent users from running cht upload-app-settings in non-standard versions, I can remove that undefined check.

If you're looking for previous isntances where CHT version was not a valid semver, you can see these on this (pvt) scraper output.

kennsippell

Leave it to you to decide whether to add the last automated tests.

kennsippell · 2024-01-10T02:01:24Z

test/fn/upload-app-settings.spec.js

@@ -20,6 +20,10 @@ describe('upload-app-settings', () => {

    // case when testing on dev
    { coreVersion: '4.2.0-dev.1682192676689', isDeclarative: true, expectNools: false },
+    // https://github.com/medic/cht-conf/issues/596
+    { coreVersion: '4.5.0.6922454971', isDeclarative: false, expectNools: false},


This isn't a super obvious requirement, so if you want it to not break in the future - it is typically nice to have a test which captures the requirement. Ya - that can be a unit test for getApiVersion or an integration test here (change what is mocked to be deeper).

1yuv · 2024-01-10T22:25:40Z

Oops, missed a colon again while merging, sending another PR to release @kennsippell , @jkuester

medic-ci · 2024-01-10T23:04:17Z

🎉 This PR is included in version 3.21.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

#596 fix issue with semver validation for latest versions

1a1cc09

1yuv requested review from jkuester and kennsippell January 5, 2024 19:45

kennsippell requested changes Jan 6, 2024

View reviewed changes

#596 add uunittest, update function call

9b56f91

kennsippell reviewed Jan 8, 2024

View reviewed changes

jkuester approved these changes Jan 8, 2024

View reviewed changes

1yuv requested a review from kennsippell January 8, 2024 19:25

#596 add more tests

7c5df2f

kennsippell approved these changes Jan 10, 2024

View reviewed changes

1yuv mentioned this pull request Jan 10, 2024

/api/deploy-info.version is not semver valid for final releases medic/cht-core#8790

Closed

1yuv merged commit bf7f647 into main Jan 10, 2024
12 checks passed

1yuv deleted the 596_semver_fix branch January 10, 2024 21:43

1yuv mentioned this pull request Jan 10, 2024

fix(#596): fix issue with semver #598

Merged

medic-ci added the released label Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#596) fix issue with semver validation for latest versions #597

fix(#596) fix issue with semver validation for latest versions #597

1yuv commented Jan 5, 2024

kennsippell left a comment

kennsippell Jan 6, 2024 •

edited

Loading

1yuv Jan 8, 2024

kennsippell Jan 8, 2024 •

edited

Loading

1yuv Jan 8, 2024

kennsippell Jan 8, 2024

1yuv Jan 8, 2024

kennsippell Jan 8, 2024 •

edited

Loading

1yuv Jan 8, 2024

kennsippell Jan 10, 2024

jkuester left a comment

1yuv commented Jan 9, 2024

kennsippell left a comment

kennsippell Jan 10, 2024

1yuv commented Jan 10, 2024

medic-ci commented Jan 10, 2024

fix(#596) fix issue with semver validation for latest versions #597

fix(#596) fix issue with semver validation for latest versions #597

Conversation

1yuv commented Jan 5, 2024

Description

Code review items

License

kennsippell left a comment

Choose a reason for hiding this comment

kennsippell Jan 6, 2024 • edited Loading

Choose a reason for hiding this comment

1yuv Jan 8, 2024

Choose a reason for hiding this comment

kennsippell Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

1yuv Jan 8, 2024

Choose a reason for hiding this comment

kennsippell Jan 8, 2024

Choose a reason for hiding this comment

1yuv Jan 8, 2024

Choose a reason for hiding this comment

kennsippell Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

1yuv Jan 8, 2024

Choose a reason for hiding this comment

kennsippell Jan 10, 2024

Choose a reason for hiding this comment

jkuester left a comment

Choose a reason for hiding this comment

1yuv commented Jan 9, 2024

kennsippell left a comment

Choose a reason for hiding this comment

kennsippell Jan 10, 2024

Choose a reason for hiding this comment

1yuv commented Jan 10, 2024

medic-ci commented Jan 10, 2024

kennsippell Jan 6, 2024 •

edited

Loading

kennsippell Jan 8, 2024 •

edited

Loading

kennsippell Jan 8, 2024 •

edited

Loading