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
service: reduce cycling refs #491
Conversation
2c60978
to
c7fed71
Compare
good to review |
Can you explain why there was a *Sevice in the first place? Because for me the serviceName (previous service) should be completely removed from task/event struct. Why Task/Event have to have knwoleage of service they are in? (Besides logs ofc) If the logs are only reason, then info about task/event services should be added somewhere else, not on struct level |
yes, but besides errors/logs?
So if for errors, then it should be removed, and error should be without ServiceName, or the service name should be added by the code is ok, but changing from *Service to string is cosmetic, it should be removed at all. |
@krhubert I understand but the change is not for cosmetic reasons. we had these cycling references before between Service type and others and now we don't keep pointers around. I don't quite agree on removing this meta data because they provide more detailed info about the errors. We can also get these meta info from the caller's level but it'll require us to create an additional error to combine all info about the error. /cc @mesg-foundation/core any more feedbacks? |
I agree that these structures shouldn't be aware of their "parent" structure especially if they don't need anything from their parent for their business logic. A structure should be self contained as much as possible and I agree that it adds some context for the errors, context that we need but the question to ask is who is responsible for these errors is it the task itself or for example the api that has the wrong parameters and so make this task invalid. The task should just say if it's valid or no and no more. The api in this example should provide the detail and the context of the error (even if we need to wrap the task error into another error). I'm totally pushing to remove these kind of dependencies that I feel are not needed and might create a lot of problem to maintain the product. I feel it's always good to avoid this kind of situation where we have a double dependency and keep it only if there is really no way we can avoid that (even this I'm sure there is always ways to avoid that). Also I think (but didn't try yet) that you introduced a bug in the In the end I'm more than agree that the goal for that is not to pass the name instead of the whole service/task but instead really remove the notion of service/task where we don't need it. |
about the removed lines from fromService() it should not introduce any bugs, they are just forgotten there, Getters sets the needed values. tests are also passes, if you see any issues please let me know. for keeping the parent's meta data info in the child, i think this is a grey area, even std packages introduces this kind of stuff. there should be a balance between where to do this or not. to me, this one is not "evil" and ok. but i'm ok with the both ways. we can combine the errors in the caller's side but we should not modify the returned error |
I'm not sure I really understand the part where it's removed from the What happen if we never call the getters ? In this case we will not have the service/serviceName. This can be a bit dangerous no ? |
the whole idea of exposing service definition from Service type is a bit ambiguous to me. I'd rather having a Definition struct and keep it under Service type privately. While refactoring service package, I didn't choose do this to reduce the complexity of PR and leave it as it is to experiment with it a bit and later decide for an improvement. I was thinking about having a Definition type where it has all these service definition and keeps the values without using pointers, this way it also prevents cycling references. we don't have any piece of logic that depends on accessing Service definitions directly instead of using Getters and I think that will never change. I agree with @antho1404 that it's a bit ambiguous to not have serviceName when you directly access it from Service type's fields. lets discuss what direction we should go with, I suggest the first option below:
with the first option we can also get rid of cycling ref for Dependency type too. |
I'm not sure to understand. I suggest to get rid of any reference ( Example with the unmarshal function in database package that return a new error rather than the one returned from |
by Definition type i'm talking about moving some parts of Service type to a new type: current: type Service struct {
ID string `hash:"-"`
Name string `hash:"name:1"`
Description string `hash:"name:2"`
Tasks []*Task `hash:"name:3"`
Events []*Event `hash:"name:4"`
configuration *Dependency `hash:"-"`
Dependencies []*Dependency `hash:"name:5"`
Repository string `hash:"name:6"`
DeployedAt time.Time `hash:"-"`
statuses chan DeployStatus `hash:"-"`
tempPath string `hash:"-"`
docker *container.Container `hash:"-"`
} after: type Service struct {
ID string `hash:"-"`
DeployedAt time.Time `hash:"-"`
def Definition `hash:"definition"`
statuses chan DeployStatus `hash:"-"`
tempPath string `hash:"-"`
docker *container.Container `hash:"-"`
}
type Definition struct {
Name string `hash:"name:1"`
Description string `hash:"name:2"`
Tasks []Task `hash:"name:3"`
Events []Event `hash:"name:4"`
Configuration Dependency `hash:"-"`
Dependencies []Dependency `hash:"name:5"`
Repository string `hash:"name:6"`
}
// FromDefinition. /cc @NicolasMahe
//
// This one can be used while creating tests otherwise we need to use New
// all the time and provide a tarball.
//
// Or we can completely lose this and have a helper to create a tarball from Definition type
// and use it with New in tests.
service.FromDefinition(Definition{
Tasks: []Task{},
})
// To save service into database we might need to create custom Marshaler to save info in def field,
// otherwise it'll be ignored by json package because it's private.
// And it seems we need to still keep FromService -or an equivalent if we change
// the way how Service saved to database- (/cc @NicolasMahe) to upgrade Service
// read from database please not that Definition does not hold any pointer values (prevents cycling refs).
|
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.
This is a good first step but just as a reminder it should not even be any notion of service in this
depends on #447
closes #437