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

fix version check on create platformatic #624

Merged
merged 2 commits into from Jan 26, 2023
Merged

fix version check on create platformatic #624

merged 2 commits into from Jan 26, 2023

Conversation

malforsaja
Copy link
Contributor

No description provided.

@mcollina
Copy link
Member

Can you please fix the DCO metadata?

@malforsaja
Copy link
Contributor Author

Why is failing again the DCO metadata?

@marcopiraccini
Copy link
Contributor

@malforsaja because you need to sign-off all the commits.
Or squash them in one commit with the sign-off

@mcollina
Copy link
Member

@malforsaja
Copy link
Contributor Author

thank you @marcopiraccini & @mcollina, seems that last option worked to solve the DCO

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@marcopiraccini marcopiraccini left a comment

Choose a reason for hiding this comment

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

Code looks good, however I think we should have a test for the case that was failing (and fixed with this PR). See the comment.

not(supportedVersions.length, 0)
test('minimumSupportedNodeVersions', async ({ equal, not }) => {
equal(Array.isArray(minimumSupportedNodeVersions), true)
not(minimumSupportedNodeVersions.length, 0)
})

test('isCurrentVersionSupported', async ({ equal }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test with the case that was failing? If I understand correctly, this is basically the old test (which wasn't enough to spot the problem that this PR is fixing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcopiraccini you're right, let me add another test for that case, because previous logic is that if we compare major versions if fails, i.e 16.18.0 should pass but when compared to 18.8.0 it fails because is less than last one.

malforsaja and others added 2 commits January 26, 2023 00:22
…ub.com>

Signed-off-by: Malfor Saja malfor.saja@gmail.com
Signed-off-by: Malfor Saja <malfor.saja@gmail.com>
Signed-off-by: Malfor Saja malfor.saja@gmail.com

Signed-off-by: Malfor Saja <malfor.saja@gmail.com>
Copy link
Contributor

@marcopiraccini marcopiraccini left a comment

Choose a reason for hiding this comment

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

lgtm! Tnx!

@mcollina mcollina merged commit 951d682 into platformatic:main Jan 26, 2023
HassanBahati pushed a commit to HassanBahati/platformatic that referenced this pull request Jan 29, 2023
* DCO Remediation Commit for Malfor Saja <malforsaja@users.noreply.github.com>

Signed-off-by: Malfor Saja malfor.saja@gmail.com
Signed-off-by: Malfor Saja <malfor.saja@gmail.com>

* add tests for major version comparison
Signed-off-by: Malfor Saja malfor.saja@gmail.com

Signed-off-by: Malfor Saja <malfor.saja@gmail.com>

Signed-off-by: Malfor Saja malfor.saja@gmail.com
Signed-off-by: Malfor Saja <malfor.saja@gmail.com>
Signed-off-by: HassanBahati <mukisabahati@gmail.com>
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.

None yet

3 participants