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

Stop depending on old oclif libs and migrate to @oclif/core directly #2671

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

otaviojacobi
Copy link
Contributor

@otaviojacobi otaviojacobi commented Sep 5, 2023

Change-type: major

Context on this PR: We are currently using @oclif/command @oclif/config and @oclif/parser libs which were all deprecated in favor of @oclif/core there are several things we get from bumping our version such as improved CLI performance and a much much better support to Typescript (which allowed us to drop several interfaces we had to manually keep).

This PR moves us away from using deprecated code. 99% of it is done following this migration guide which changes flags to Flags and the whole structure of Arguments.

The patches we did on @oclif/parser have been ported to @oclif/core patch. The patch on @oclif/config could be dropped as this was fixed upstream and also we could also remove one patch on oclif itself as support for windows --targets build was fixed on upstream oclif/oclif#1181 too.

There are no changes to any of the tests. There is one small change in order to make the tests run with the new @oclif/core format here


Please check the CONTRIBUTING.md file for relevant information and some
guidance. Keep in mind that the CLI is a cross-platform application that runs
on Windows, macOS and Linux. Tests will be automatically run by balena CI on
all three operating systems, but this will only help if you have added test
code that exercises the modified or added feature code.

Note that each commit message (currently only the first line) will be
automatically copied to the CHANGELOG.md file, so try writing it in a way
that describes the feature or fix for CLI users.

If there isn't a linked issue or if the linked issue doesn't quite match the
PR, please add a PR description to explain its purpose or the features that it
implements. Adding PR comments to blocks of code that aren't self explanatory
usually helps with the review process.

If the PR introduces security considerations or affects the development, build
or release process, please be sure to highlight this in the PR description.

Thank you very much for your contribution!

@otaviojacobi otaviojacobi changed the title Remove direct dependency to @oclif/config Stop depending on old oclif libs and migrate to @oclif/core directly Sep 5, 2023
docs/balena-cli.md Outdated Show resolved Hide resolved
@otaviojacobi otaviojacobi force-pushed the stop-depending-on-old-oclif-libs branch 4 times, most recently from efbfbf0 to 5e25d7b Compare September 6, 2023 17:23
@@ -26,26 +27,6 @@ import {
} from '../../utils/messages';
import type { BalenaSDK, PineDeferred } from 'balena-sdk';

interface FlagsDef {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thgreasi @myarmolinsky This is one example of one of the big wins in this new version. the static "flags" types now are inffered from what you added on your flags so you get type checking working fine on your flags ("options") without manually needing to cast to what are the types of the flags

help: void;
service?: string; // service name
}
type FlagsDef = Interfaces.InferredFlags<typeof EnvsCmd.flags>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thgreasi @myarmolinsky Also here is one example in case we need to create a function that receives the parsed type of the flags we can use the new (and very userfull) Interfaces.InferredFlags instead of manually mantaining types, we let oclif do the dirty job for us.

@otaviojacobi otaviojacobi force-pushed the stop-depending-on-old-oclif-libs branch 4 times, most recently from 88048c5 to 7900b3c Compare September 6, 2023 19:03
@otaviojacobi otaviojacobi marked this pull request as ready for review September 7, 2023 12:36
@@ -78,6 +78,6 @@ export async function disambiguateReleaseParam(
/**
* Convert to lowercase if looks like slug
*/
export function lowercaseIfSlug(s: string) {
export async function lowercaseIfSlug(s: string) {
Copy link
Member

Choose a reason for hiding this comment

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

Do custom parsers need to be async and fail otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. The new interface expects the return of a parse function to be a Promise

Copy link
Member

@thgreasi thgreasi left a comment

Choose a reason for hiding this comment

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

looks fine 👍

@otaviojacobi
Copy link
Contributor Author

LGTM

@flowzone-app flowzone-app bot merged commit 8e6a6b8 into master Sep 26, 2023
53 checks passed
@flowzone-app flowzone-app bot deleted the stop-depending-on-old-oclif-libs branch September 26, 2023 13:56
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

2 participants