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

Update node resolver #47

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update node resolver #47

wants to merge 4 commits into from

Conversation

kb2ma
Copy link
Contributor

@kb2ma kb2ma commented Feb 2, 2024

Node resolver was using obsolete 'resin' repository; update to use 'balenalib'.

Also included a whitespace fix for prettier so can run tests.

@kb2ma kb2ma added the bug Something isn't working label Feb 2, 2024
@flowzone-app flowzone-app bot enabled auto-merge February 2, 2024 01:16
@kb2ma kb2ma requested review from dfunckt and klutchell February 2, 2024 01:17
@kb2ma
Copy link
Contributor Author

kb2ma commented Feb 2, 2024

@klutchell , @dfunckt This fix is simple. I rarely work in this area, so want to confirm I'm not missing any context that would affect the change. I'm scratching my head because the resin repository was deprecated 5 years ago. It seems like we would have noticed this error sooner.

@klutchell
Copy link
Collaborator

klutchell commented Feb 2, 2024

@klutchell , @dfunckt This fix is simple. I rarely work in this area, so want to confirm I'm not missing any context that would affect the change. I'm scratching my head because the resin repository was deprecated 5 years ago. It seems like we would have noticed this error sooner.

We are optionally skipping that test, which is why we didn't notice it sooner. I expect that's actually WHY we are skipping that test.

@klutchell
Copy link
Collaborator

I suggest we remove the .skip from this test to make sure the test is marked as required

it.skip('should resolve a nodeJS project', function () {

@dfunckt
Copy link
Collaborator

dfunckt commented Feb 2, 2024

This is the offending commit: balena-io-modules/resin-bundle-resolve@938f5e3

@kb2ma
Copy link
Contributor Author

kb2ma commented Feb 2, 2024

Thanks, I'll activate the test, confirm it has the expected result, and then self-approve.

@klutchell
Copy link
Collaborator

I would be happy to review and approve once the test is re-enabled. No need to self-certify today :)

Change-type: patch
Signed-off-by: Ken Bannister <[email protected]>
Change-type: patch
Signed-off-by: Ken Bannister <[email protected]>
Also expect that the (final) result from retrieving node image tags
may be undefined.

Change-type: patch
Signed-off-by: Ken Bannister <[email protected]>
Also update expected node version for use with balenalib repo on
Docker Hub.

Signed-off-by: Ken Bannister <[email protected]>
@kb2ma kb2ma force-pushed the update-node-resolver branch from 1617f4d to 2072920 Compare February 11, 2024 03:31
@kb2ma
Copy link
Contributor Author

kb2ma commented Feb 11, 2024

Reactivated the node project test. I had to update the node version to one available in the balenalib repository. Also I found that the result from querying Docker Hub might be undefined. The possibility of that error may have been the reason for skipping the test. The test pretty consistently takes 40 sec., which also might be why the test was skipped.

@klutchell
Copy link
Collaborator

Also I found that the result from querying Docker Hub might be undefined.

I suspect the test was skipped due to the undefined result. Does this fail the test? Do you think this is also affecting users in production (if anyone even deploys without Dockerfiles anymore)?

@flowzone-app flowzone-app bot closed this in #48 Feb 13, 2024
auto-merge was automatically disabled February 13, 2024 13:26

Pull request was closed

@Page- Page- reopened this Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants