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

Backport v0.9 various fixes to v0.8 #72

Closed
wants to merge 12 commits into from

Conversation

dr-orlovsky
Copy link
Contributor

@dr-orlovsky dr-orlovsky commented Jan 17, 2023

As requested by @zoedberg in #65

@dr-orlovsky dr-orlovsky added the bug Something isn't working label Jan 17, 2023
@dr-orlovsky dr-orlovsky added this to the v0.8.x milestone Jan 17, 2023
@dr-orlovsky dr-orlovsky mentioned this pull request Jan 17, 2023
@dr-orlovsky
Copy link
Contributor Author

@zoedberg can you pls accept invitation to this org such that I can assing a review to you?

@zoedberg
Copy link
Contributor

sure, just accepted. @dr-orlovsky

@dr-orlovsky dr-orlovsky requested a review from zoedberg January 17, 2023 12:32
Copy link
Contributor

@zoedberg zoedberg left a comment

Choose a reason for hiding this comment

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

tested by cherry-picking on top of this branch the commit updating clap, works good, thanks @dr-orlovsky

@dr-orlovsky
Copy link
Contributor Author

dr-orlovsky commented Jan 17, 2023

Looking at all these changes I have a serious doubt whence we need to do this "backporting". Most of them for sure are API-breaking etc. It will take a lot of time to review. Why to invest time into that if we have v0.9 and those needing this just need to adopt a new version?

Anyway wait for your opinion and review @zoedberg. I would not spend my time into sorting out what of this is a breaking change...

@zoedberg
Copy link
Contributor

For us breaking changes at the API level are not an issue because we’re still in an early phase of lnp-node integration. But your observation is correct, it may be a problem for devs that already started integrating that (for example I think diba is already integrating lnp-node, right @cryptoquick?), so we probably should not backport everything but only non breaking changes (if any)

@dr-orlovsky
Copy link
Contributor Author

dr-orlovsky commented Jan 17, 2023

I think that overall it should be nice to stick to the semver requirements than to rely on an assumption that nobody uses this. So is there a reason to backport (potentially breaking) new features from v0.9 to v0.8 if we already have v0.9 released?

Tagging @nicbus as one of the main proponents of semver which I know :D

@nicbus
Copy link

nicbus commented Jan 18, 2023

I think we all agree that sticking to semver and not having braking changes in a minor/patch release is best. therefore, as @zoedberg was also saying, what's desirable is to have only non-breaking changes backported into v0.8, but I also see your point about this requiring some time to review each change to assess if it has to be excluded

in the end, I think there are no problems in leaving these fixes out of v0.8 for the above reasons but, before we choose to do so, I'd wait for a go from @cryptoquick, as diba might need some of these

@cryptoquick
Copy link

We're fully updated to 0.9 here at DIBA, so I don't have a horse in this race.

@zoedberg
Copy link
Contributor

ok so I guess we can close this PR without merging it and release a new lnp-node with just clap update @dr-orlovsky

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.

5 participants