-
Notifications
You must be signed in to change notification settings - Fork 13
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
Marketplace publish service raw data #845
Conversation
12e1b5e
to
f2ef919
Compare
f2ef919
to
afd2380
Compare
pull request updated without the update on the image tag so ready to review cc @mesg-foundation/core |
922e77d
to
9b65307
Compare
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.
can you express why we introduce xxx-service
?
Because I did a wrong merge to resolve some conflicts and actually took some local files that I didn't wanted to commit. Sorry for that, it's reverted |
if err != nil { | ||
return err | ||
} | ||
fmt.Printf("%s Service deployed with sid %s and hash %s\n", pretty.SuccessSign, pretty.Success(c.service.Definition.Sid), pretty.Success(c.service.Definition.Hash)) |
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.
it might be better to explicitly use hash
got from deployService()
here. because in future, we'll make service sids to have one-to-many relationships with service hashes.
thus, ServiceByID()
may return multiple versions and maybe in desc order on deploy time but it's not guaranteed that the first hash in the array will be belong to the last publish because two publish commands may run in parallel for the different versions of same service.
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.
if ServiceByID(**sid**)
is only going to return the last service version and ServiceByID(**hash**)
will only return the same version with hash
, then all ok.
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.
anyway, we're also using c.service.Definition
so we have to make this call. let's make sure that ServiceByID(hash)
will return the version for hash
in future too and ServiceByID(sid)
may return the latest hash.
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.
if
ServiceByID(**sid**)
is only going to return the last service version andServiceByID(**hash**)
will only return the same version withhash
, then all ok.
Yeah, that's the case.
anyway, we're also using
c.service.Definition
so we have to make this call. let's make sure thatServiceByID(hash)
will return the version forhash
in future too andServiceByID(sid)
may return the latest hash.
Yep
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.
manual tests are ok
Yeah that's will be nice |
The marketplace was sending the content of the
mesg.yml
as definition, I feel it's better to have the data that has been processed (the ones directly in the database after de deployment) like that if we want we can have another source of validation and also themesg.yml
can always be compiled down to these data, the other was is not necessarily possible.With this PR I also realized that
hash
andhashVersion
may not be necessary in the marketplace manifest (https://github.com/mesg-foundation/core/blob/12e1b5e61aa664dc33678a155c24994489868c75/commands/marketplace_publish.go#L91-L92)hash
is present in the raw datahashVersion
is something that should be present in the raw dataThis is for another PR but I would like to remove this from the marketplace and move the
hashVersion
that I would call justversion
(for service version) directly in the core.@ilgooz this also changes the website because raw data uses arrays for the parameters and mesg.yml uses maps