-
Notifications
You must be signed in to change notification settings - Fork 216
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 ff cli to main #1524
update ff cli to main #1524
Conversation
Signed-off-by: Enrique Lacal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking back and I don't think take this approach recently @EnriqueL8
So probably needs thinking through and comparing to how we've done it with releases in the past.
(even if the only change is to the PR comment to describe how we've managed this in the past, the implications, and how we're planning to do it going forwards)
The Given that this file is in main I think we should be keeping a reference to the main components especially the FireFly CLI if not we would constantly be releasing new version of the sub components too keep up to date with it and it will become inconsistent with the version of FireFly. I could release a new FireFly CLI We use this manifest.json in a couple of ways:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly disagree with using an indeterminate reference for anything in the manifest. If I run a build on Tuesday, and retry the exact same build on Wednesday, it could be using a completely different version. That's bad - the point of the manifest is reproducibility.
We have always either used tagged releases or specific builds generated from main. So we don't have to do a release of the CLI, but we do need to point at one specific version rather than just "main".
In terms of versioning in general:
|
Makes sense - will point it a specific commit of the FireFly CLI and I couldn't agree more with
|
Signed-off-by: Enrique Lacal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems OK to me. For the record, I see that you're pointing at https://github.com/hyperledger/firefly-cli/commits/1378d838891cd0a43d9a255dac9b707a69f13c59, which corresponds to the point where hyperledger/firefly-cli#308 was merged.
Yes, thought it would pick that fix as well |
No description provided.