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

Add dependency management #32

Merged
merged 5 commits into from
Jul 12, 2022
Merged

Add dependency management #32

merged 5 commits into from
Jul 12, 2022

Conversation

shamsartem
Copy link
Collaborator

No description provided.

@linear
Copy link

linear bot commented Jul 7, 2022

DXJ-73 Ability to override aqua cli version

fluence dependency aqua --use recommended (will use hardcoded version. Used by default)

fluence dependency aqua --use 0.1.2-323 (will use the version that you provided)

fluence dependency aqua --version (return current version)

fluence dependency --use recommended (all dependencies will be stable)

store this info at ~/.fluece/dependencies.yaml

also remove download confirmation and replace message with

Installing @fluencelabs/aqua v0.3.7 to ~/.fluence :spinner:

@shamsartem shamsartem self-assigned this Jul 11, 2022
@shamsartem shamsartem requested a review from coder11 July 11, 2022 13:20
@@ -43,3 +43,11 @@ export const assertHasKey: <K extends string>(
throw new AssertionError({ message });
}
};

export const getIsStringUnion =
Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide a comment explaining what does this function do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will provide a comment and I know a way to make it look a bit better

src/lib/npm.ts Outdated
{ recommendedVersion: string; bin: string; packageName: string }
> = {
[AQUA_NPM_DEPENDENCY]: {
recommendedVersion: "0.7.4-322",
Copy link
Contributor

Choose a reason for hiding this comment

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

What is our policy regarding aqua version? What if a new version, say 0.7.4-323 gets released? Should we update it here as well and bump the version of Fluence CLI? Or should we consolidate changes? Does it also mean that Fluence CLI version will depend on version(s) of it's deps?

Copy link
Collaborator Author

@shamsartem shamsartem Jul 11, 2022

Choose a reason for hiding this comment

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

So basically I wanted our users to have a predictable experience so I decided to hard-code aqua version that I checked is working correctly with Fluence CLI and also give user this command fluence dependency which allows to override recommended version of the dependency

The idea is we allow aqua cli developers to do whatever breaking changes they want and we need to check if the new version is working and update this version each time we release. I think it's safer to do it like that, but user still
has flexibility to switch aqua cli version if he wants to

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable. I still think It would be a good idea to extract the version string into some dedicated file, so it would be easier to spot the exact place where dependencies are specified. Once the dependencies are separated we would be able to live with such structure for a while and see if it would be possible to improve.

Copy link
Contributor

@coder11 coder11 left a comment

Choose a reason for hiding this comment

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

Overall looks good. Let's extract the aqua dependency version into a separate place as mentioned in the comment.

@shamsartem shamsartem merged commit ce156fb into main Jul 12, 2022
@shamsartem shamsartem deleted the DXJ-73 branch July 12, 2022 08:12
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.

2 participants