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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Jan 10, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a1cc09
#596 fix issue with semver validation for latest versions
1yuv Jan 5, 2024
9b56f91
#596 add uunittest, update function call
1yuv Jan 8, 2024
7c5df2f
#596 add more tests
1yuv Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/fn/upload-app-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ async function augmentDeclarativeWithNoolsBoilerplate(appSettings) {
return;
}

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

@kennsippell kennsippell Jan 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@kennsippell kennsippell Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if (addNoolsBoilerplate) {
appSettings.tasks.rules = nools.addBoilerplateToCode(appSettings.tasks.rules);

Expand Down
2 changes: 1 addition & 1 deletion src/lib/get-api-version.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const getApiVersion = async () => {
};

const getValidApiVersion = async () => {
return semver.valid(await getApiVersion());
return semver.valid(semver.coerce(await getApiVersion()));
kennsippell marked this conversation as resolved.
Show resolved Hide resolved
};

module.exports = { getValidApiVersion };
4 changes: 4 additions & 0 deletions test/fn/upload-app-settings.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@kennsippell kennsippell Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

// non-declarative < 4.2
{ coreVersion: '4.1.0.6922454971', isDeclarative: undefined, expectNools: false}
];

for (const scenario of scenarios) {
Expand Down