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

Support optional version and tag #38

Merged
merged 7 commits into from
Jan 14, 2023
Merged

Support optional version and tag #38

merged 7 commits into from
Jan 14, 2023

Conversation

stuartmscott
Copy link
Member

@stuartmscott stuartmscott commented Jan 12, 2023

State Machines identities can now include an optional version to enable multiple versions of the same binary and tag to distinguish multiple instances of the same binary;

  • package/name@address
  • package/name:tag@address
  • package/nameversion@address
  • package/nameversion:tag@address

Fixes: #11

@winksaville
Copy link
Collaborator

My first reaction is that it's overly complex. And makes a name with an "_v" in it potentially ambiguous. I'd suggest defining a way to add version to a tag unambiguously. Maybe with another separator a suggestion might be ";":

name:tag;version@address

And then to keep the parsing logic simple only one ":", ";" and "@" are allowed in the "connection string".

One other thing, I'd assume an single actor should be able to handle messages defined for a set of "tag/version" pairs. So, it would seem that information needs to be available to the message handler on a message by message basis. Maybe you're handling that, I haven't looked at the code yet :)

@stuartmscott
Copy link
Member Author

You're right it could be simplified - the _v wasn't really necessary and can be removed.

If a server is asked to spawn a machine and that file exists, start it. If the file doesn't exist but some files do exist with the same name plus a semantic version suffix, then choose the latest version.

winksaville
winksaville previously approved these changes Jan 13, 2023
Copy link
Collaborator

@winksaville winksaville left a comment

Choose a reason for hiding this comment

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

This is a huge change and it's tough to do a review so it's just "skimming".

One thing I see is that your class SemVer and semver.org supports 5 fields (major, minor, patch, prerelease, build) but it appears you've documented that a filename there is :@

and your examples imply is only (major, minor, patch) is that correct?

I was looking at resolveVersion and the binary /= machine; statement had me scratching my head. I looked it up and apparently its a concatenation of two strings with the appropriate path separator, tricky :)

Again, just a quick review.

test/src/semver.cpp Outdated Show resolved Hide resolved
@stuartmscott
Copy link
Member Author

I think the formatting in your message got messed up ":@" - same thing happened to my description above.

SemVer doesn't include ':' or '@', but there is still more work needed to support the prerelease and build segments.

Yeah the std::filesystem::path concatenation is neat, albeit a little unintuitive.

Appreciate the review!

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.

Reconsider using bin-name+network-address as the Unique ID
2 participants