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 balena-sdk to 15.51.1 #2346

Merged
1 commit merged into from
Sep 22, 2021
Merged

Update balena-sdk to 15.51.1 #2346

1 commit merged into from
Sep 22, 2021

Conversation

nitishagar
Copy link
Contributor

@nitishagar nitishagar commented Sep 20, 2021

Copy link
Contributor

@pdcastro pdcastro left a comment

Choose a reason for hiding this comment

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

Looks good @nitishagar 👍   Just a minor thing: The commit and PR specify change-type: minor, but from what I can tell, no new functionality is provided to CLI users. It looks to me like it fits in the category of "chore" dependency updates with no impact to end users, also like bug fixes or security updates, which I would normally classify as a "patch" (rather than "minor") update.
If there is actually some new feature provided to users, then the commit message could perhaps be amended to reflect it (while keeping the commit body as it is).

@klutchell
Copy link
Contributor

I'm a little wary that the following unrelated upstream changes would also be included:

These were supposed to be done around the same time as some other fixes for #2152. However, we haven't figured out those other fixes yet and the PR mentioned above actually masks the Z_BUF_ERR and appears to download the image successfully even though the final file is incomplete.

So while on it's own it seems like a good fix to have, it actually hides another issue that we haven't solved yet. I can revert balena-requests, or revert the change in both repos while moving them forward, but I worry we will start seeing incomplete file downloads unless we get to the root of the os download pipeline issues.

Thoughts on how to proceed and unblock this PR?

@pdcastro
Copy link
Contributor

[...] and the PR mentioned above actually masks the Z_BUF_ERR and appears to download the image successfully even though the final file is incomplete.
[...] I can revert balena-requests, or revert the change in both repos while moving them forward [...]

Thanks for the insight @klutchell 👍  Instead of reverting those changes, could the changes be enabled/disabled through a feature switch? If we could think of no better option, perhaps even an environment variable that the CLI could set, starting with this PR, say process.env.MASK_Z_BUF_ERR=false, either globally or in selected commands like balena os download. (I don't mean an environment variable that users would see/set, I mean simply CLI code assigning to process.env.VAR_NAME.)

@nitishagar
Copy link
Contributor Author

For the following bit:

If there is actually some new feature provided to users, then the commit message could perhaps be amended to reflect it (while keeping the commit body as it is).

Among other changes, we are looking to enable addition of provision-key name as a param for CLI to consume. Ref: #2294

I took the default message with the update script, but can detail it based on the above. Let me know.

@pdcastro
Copy link
Contributor

If there is actually some new feature provided to users [...]

Among other changes, we are looking to enable addition of provision-key name as a param for CLI to consume. Ref: #2294

@nitishagar, but this PR (just bumping balena-sdk) does not add provision-key name as a param. I understand that this PR does not make any difference to CLI users (other than possibly introducing a bug in balena os download as mentioned by Kyle). In other words, I understand that there isn't anything that CLI users can do with the CLI after this PR is merged, that they could not do before it was merged. So this PR would be Change-type: patch, and #2294 would be Change-type: minor.

I took the default message with the update script, but can detail it based on the above. Let me know.

If my understanding above is correct, then the commit message is OK as it is, but Change-type should be patch.

The reason why it matters (a little bit) is that the commit message and new semver version will be visible in the Changelog that CLI end users and support agents refer to in order to find out what's new, and to decide whether to update the CLI.

@klutchell
Copy link
Contributor

I would like to get the following PRs merged before this one if possible:

package.json Outdated
@@ -207,7 +207,7 @@
"balena-image-manager": "^7.0.3",
"balena-preload": "^10.5.0",
"balena-release": "^3.2.0",
"balena-sdk": "^15.48.0",
"balena-sdk": "^15.51.0",
Copy link
Contributor

@pdcastro pdcastro Sep 21, 2021

Choose a reason for hiding this comment

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

@nitishagar, Kyle's PRs (including #2347) have been merged and balena-sdk v15.51.1 is now available. Please select balena-sdk v15.51.1 here, change the commit Change-type to patch and rebase on master, then we can get this PR merged. Thanks! 🙏

Update balena-sdk from 15.48.0 to 15.51.1

Change-type: patch
@nitishagar nitishagar changed the title Update balena-sdk to 15.51.0 Update balena-sdk to 15.51.1 Sep 22, 2021
@ghost ghost merged commit 8b9e3cc into master Sep 22, 2021
@ghost ghost deleted the update-sdk branch September 22, 2021 14:50
This pull request was closed.
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