KomodoPlatform / komodo-defi-framework

This is the official Komodo DeFi Framework repository
https://komodoplatform.com/en/docs/komodo-defi-framework/
97 stars 88 forks source link

chore(bin): rename mm2 binaries to kdf #2126

Open shamardy opened 1 month ago

shamardy commented 1 month ago

This PR renames mm2 binaries and libs to kdf

laruh commented 1 month ago

Should these paths also be renamed?

Screenshot 2024-05-24 at 19 19 01

shamardy commented 1 month ago

Should these paths also be renamed?

Thanks for finding these, I am not sure they are used but will rename them.

borngraced commented 1 month ago

What about renaming mm2_* crates name too?

laruh commented 1 month ago

What about renaming mm2_* crates name too?

I think it will be a rabbit hole for this pr :D wallet team asked for the binaries at least. But its up to Omer

shamardy commented 1 month ago

What about renaming mm2_* crates name too?

We will do the renaming gradually whenever a need for renaming arises, this way we don't create much conflicts in current open PRs.

shamardy commented 1 month ago

Making this change will break many deployment setups. We should make sure people (specially notary node and our devops people) aware that we are going to rename the binary.

Already informed @KomodoPlatform/qa to update docs and they will be responsible for informing other people. I will include the breaking changes in the release notes too, should we change next release to v3.0.0-beta to follow semantic versioning?

laruh commented 1 month ago

should we change next release to v3.0.0-beta to follow semantic versioning

yes, def should do it. as its breaking change and its not stable, then 3 0 0 beta version should be

onur-ozkan commented 1 month ago

Providing a copy with mm2 name next to kdf binary would be fine for a while (~3 months). I don't think we should bump the major version just because we renamed the binary (it's breaking change for deployments not the application logic).

laruh commented 1 month ago

https://semver.org/#spec-item-8

Major version X (X.y.z | X > 0) MUST be incremented if any backward incompatible changes are introduced to the public API. It MAY also include minor and patch level changes. Patch and minor versions MUST be reset to 0 when major version is incremented.

additional source https://www.infoq.com/articles/breaking-changes-are-broken-semver/

As I see any breaking changes that can cause existing systems or scripts to fail (changes in binary names, config parameters, or deployment paths, as these can disrupt automated processes and user workflows etc) should be treated as Major version changes.

onur-ozkan commented 1 month ago

https://semver.org/#spec-item-8

Major version X (X.y.z | X > 0) MUST be incremented if any backward incompatible changes are introduced to the public API. It MAY also include minor and patch level changes. Patch and minor versions MUST be reset to 0 when major version is incremented.

additional source https://www.infoq.com/articles/breaking-changes-are-broken-semver/

As I see any breaking changes that can cause existing systems or scripts to fail (changes in binary names, config parameters, or deployment paths, as these can disrupt automated processes and user workflows etc) should be treated as Major version changes.

We don't change the API that's the point. With major bump, you want to inform people that you made a breaking change, right? How are we supposed to get the version information? The binary name has been changed :) You version the application, here we don't change the application. As I said, this is breaking change for deployments not application itself.

shamardy commented 1 month ago

Providing a copy with mm2 name next to kdf binary would be fine for a while (~3 months)

This is for the release artifacts, right? we don't need to do this in CI, or did you mean that?

shamardy commented 1 month ago

As I see any breaking changes that can cause existing systems or scripts to fail (changes in binary names, config parameters, or deployment paths, as these can disrupt automated processes and user workflows etc) should be treated as Major version changes.

That's correct, but I am not sure we need this currently while we are beta, the first stable release will start at 1.0.0 again and then we will need to make sure semver is right with every new release. Usually for our beta version if we made a big change like the p2p network upgrade we would bump the major version indicating a big change otherwise we just bump the minor version.

laruh commented 1 month ago

As I see any breaking changes that can cause existing systems or scripts to fail (changes in binary names, config parameters, or deployment paths, as these can disrupt automated processes and user workflows etc) should be treated as Major version changes.

That's correct, but I am not sure we need this currently while we are beta, the first stable release will start at 1.0.0 again and then we will need to make sure semver is right with every new release. Usually for our beta version if we made a big change like the p2p network upgrade we would bump the major version indicating a big change otherwise we just bump the minor version.

So, if I understand you correctly, we will strictly follow the semver rules starting with the "stable" version, and our "beta" allows for some flexibility. This approach is fine with me, since we will be moving to the stable version soon. I don't insist on the current version as long as we have clear rules for the stable releases.

laruh commented 1 month ago

https://github.com/KomodoPlatform/komodo-defi-framework/pull/2126/commits/a1e288cccd195b18da327deb5a5ba330c75f17a8 Now we have two Dockerfiles with identical commands Screenshot 2024-05-28 at 19 30 23

But they are used in diff build.yml 🤔 Screenshot 2024-05-28 at 19 36 55

shamardy commented 1 month ago

https://github.com/KomodoPlatform/komodo-defi-framework/commit/a1e288cccd195b18da327deb5a5ba330c75f17a8 Now we have two Dockerfiles with identical commands

Let's leave them in case we ever need different commands for dev or release.