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

Add info api #737

Closed
wants to merge 0 commits into from
Closed

Add info api #737

wants to merge 0 commits into from

Conversation

antho1404
Copy link
Member

This API returns the basic configurations of the core and the list of services that are started by the core to let any client consume them

depends on #725

@antho1404 antho1404 force-pushed the feature/info-api-and-deployed-sid branch 2 times, most recently from 32e1c88 to 7996b57 Compare January 28, 2019 06:02
@antho1404 antho1404 mentioned this pull request Jan 28, 2019
1 task
@antho1404 antho1404 force-pushed the feature/info-api-and-deployed-sid branch from 7996b57 to 80fcb04 Compare January 28, 2019 06:11
@ilgooz
Copy link
Contributor

ilgooz commented Jan 28, 2019

Why clients needs to know what services the Core uses?

@antho1404
Copy link
Member Author

The CLI needs to know that in order to call the services related to the marketplace. It's a way to expand the API. We could have "private" services maybe later if needed but now it's to expose all accessible "plugins" of the core. With this the CLI or UI could access all informations needed only with the services and without the need to create an api for every single feature.

protobuf/coreapi/api.proto Outdated Show resolved Hide resolved
ilgooz
ilgooz previously approved these changes Jan 28, 2019
ilgooz
ilgooz previously approved these changes Jan 28, 2019
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved

// Info returns all necessary informations from the core.
func (s *Server) Info(ctx context.Context, request *coreapi.InfoRequest) (*coreapi.InfoReply, error) {
c, err := config.Global()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not use config.Global - pass the config or needed data

Copy link
Member Author

Choose a reason for hiding this comment

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

What would be your recommendations ?

  • Give the config as dependency of Server (we need to change the New because we give the address and this address comes from the config)
  • Delegate to the API and give the config to the API.
  • Give a struct with all the info (needed by the Info api) to the server when we create it
  • Keep it like that, config.Global is a singleton so let's use a singleton

I'm not really happy with any of them so I'm open for this but anyway this might be done in another PR.

Copy link
Contributor

@krhubert krhubert Jan 28, 2019

Choose a reason for hiding this comment

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

in short: 1st or 3rd

1st gives you less code because you don't need to create struct like grpc.ServerOptions|ServerInfo, but by using 3rd the Server (and it tests) is independent of config package. So by creating one struct with option and copy those values during init in core/main.go you drop one dependency.

About 2nd and 4th

  • 2nd let's use api for api :), relaying that API object will have config struct introduce dependency and you can't easily modify api
  • 4th using singleton (Global) is not the best - in fact the Global function should be removed (or used only in main func()). Getting config make changing, refactoring and reading package more simple. Example from our source base:

you open Service and you see service struct and on what variables it relys, but wait what is this:
1
config inside a function to get some more variables to start service. Ok I have everthing, but no wait:
2
Another get of config to get more variables.

And it's done by two different developers, so there are two options: let everyone use config.Global everywhere (mess) or the next developer check if the config.Global has been used if yes, refactor the code to get it only in one place etc... But after two week nobody will remember to check this.

So based on this, I would highly recomend option 3 as the best one.

There is also 5th option. Do not pass whole config , but

// Config contains all the configuration needed.
type Config struct {
  Server struct {
    Address string
  }

  Client struct {
    Address string
  }

  Log struct {
    Format string
    Level  string
  }

  Core struct {
    Image string
    Name  string
    Path  string

    Database struct {
      ServiceRelativePath   string
      ExecutionRelativePath string
    }
  }

  Docker struct {
    Socket string
    Core   struct {
      Path string
    }
  }
}

Split this config into chunk

// Config contains all the configuration needed.

  ServerConfig struct {
    Address string
  }

  ClientConfig struct {
    Address string
  }

  LogConfig struct {
    Format string
    Level  string
  }

  CoreConfig struct {
    Image string
    Name  string
    Path  string

    DatabaseConfg struct {
      ServiceRelativePath   string
      ExecutionRelativePath string
    }
  
  DockerConfig struct {
    Socket string
    Core   struct {
      Path string
    }
  }

type Config struct {
  Server ServerConfig
  Client ClientConfig
  // ....
}

And pass only the required config so it's something between 1 and 3.

protobuf/coreapi/api.proto Outdated Show resolved Hide resolved
@@ -42,6 +42,9 @@ service Core {

// ServiceLogs gives a stream for dependency logs of a service.
rpc ServiceLogs (ServiceLogsRequest) returns (stream LogData) {}

// Info returns all necessary informations from the core.
rpc Info(InfoRequest) returns (InfoReply) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Info/CoreInfo/ but i'm not sure about this

Copy link
Member Author

Choose a reason for hiding this comment

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

I think Info is fine, we are accessing the API of the Core already

message CoreService {
string sid = 1;
string hash = 2;
string url = 3;
Copy link
Member

Choose a reason for hiding this comment

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

Hash is useless because of sid.
The service key / type is missing.
The goal is for the cli to get the sid from a hardcoded type.
Eg: Give me SID of service type 'marketplace'.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok for the "key" but the hash is absolutely needed, the sid can be deleted from that but the hash is the version we want to use, not the sid, sid is just an alias on the latest version deployed.
Sid is useless because of hash, not the other way around

}
repeated CoreService services = 1;
string Address = 2;
string Image = 3;
Copy link
Member

Choose a reason for hiding this comment

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

Replace image by version

Copy link
Member Author

Choose a reason for hiding this comment

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

why ? ok to add another field version but this is the docker image, it's not the version. Even if there is a link between the version and the docker image let's not mix them.

string hash = 2;
string url = 3;
}
repeated CoreService services = 1;
Copy link
Member

Choose a reason for hiding this comment

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

This could be a map like the Service in config.

@antho1404 antho1404 closed this Jan 29, 2019
@antho1404 antho1404 force-pushed the feature/info-api-and-deployed-sid branch from 6bb6993 to f498c3b Compare January 29, 2019 04:15
This was referenced Jan 29, 2019
@mesg-bot
Copy link
Member

This pull request has been mentioned on MESG Community. There might be relevant details there:

https://forum.mesg.com/t/use-of-config-in-the-core/238/1

2 similar comments
@mesg-bot
Copy link
Member

This pull request has been mentioned on MESG Community. There might be relevant details there:

https://forum.mesg.com/t/use-of-config-in-the-core/238/1

@mesg-bot
Copy link
Member

This pull request has been mentioned on MESG Community. There might be relevant details there:

https://forum.mesg.com/t/use-of-config-in-the-core/238/1

@mesg-bot
Copy link
Member

mesg-bot commented Feb 1, 2019

This pull request has been mentioned on MESG Community. There might be relevant details there:

https://forum.mesg.com/t/use-of-config-in-the-core/238/4

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.

None yet

5 participants