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

Common api #37

Merged
merged 25 commits into from
May 27, 2020
Merged

Common api #37

merged 25 commits into from
May 27, 2020

Conversation

EItanya
Copy link
Member

@EItanya EItanya commented May 11, 2020

No description provided.

@solo-changelog-bot
Copy link

Issues linked to changelog:
#15


package core.skv2.solo.io;

option go_package = "github.com/solo-io/skv2/pkg/api/core.skv2.solo.io/v1alpha1/types";
Copy link
Member

Choose a reason for hiding this comment

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

not sure i agree with the choice of v1alpha1 for the version for core types.

also, what's the reasoning using a types package rather than a package named for the api version?

Copy link
Member Author

@EItanya EItanya May 18, 2020

Choose a reason for hiding this comment

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

Why no v1alpha1, using the versioning just allows us to make breaking changes to the API without having to make breaking changes to the code. As we support more APIs, and potentially multiple versions of those APIs, following kubernetes versioning for our APIs will become more and more useful.

I will delete the types package


string name = 3;

string namespace = 4;
Copy link
Member

Choose a reason for hiding this comment

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

(comments)

FAILED = 3;

// Finished processing successfully.
ACCEPTED = 4;
Copy link
Member

Choose a reason for hiding this comment

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

i'm not sure about making State a core api in skv2 yet. i think it's worth discussing at least

@@ -0,0 +1,60 @@
package main
Copy link
Member

Choose a reason for hiding this comment

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

we already have some code generation in skv2 for multicluster; maybe this belongs alongside that?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have templates to generate multi cluster reconcilers, but I don't think we have code to specifically generate code in this repo to be consumed by other repos, I may be wrong though.

*/
message KubernetesClusterSpec {
// name of the secret which contains the kubeconfig with information to connect to the remote cluster.
string secret = 1;
Copy link
Member

Choose a reason for hiding this comment

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

i'd like to allow the user to provide a secret_key here as well. we can override emtpy with a default

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 do you mean by a secret_key

Copy link
Contributor

Choose a reason for hiding this comment

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

I think he means the key of the kubeconfig entry in the secret.

kind: Secret
metadata:
  ...
data:
  <cluster.secret_key | "some-default">: encoded_kubeconfig

Comment on lines 19 to 26
// version of kubernetes
string version = 2;
// cloud provider, empty if unknown
string cloud = 3;

string region = 4;

string zone = 5;
Copy link
Member

Choose a reason for hiding this comment

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

are any of these other fields used anywhere? how are they intended to be consumed?

@EItanya EItanya marked this pull request as ready for review May 19, 2020 00:47
@joekelley joekelley mentioned this pull request May 20, 2020
@soloio-bulldozer soloio-bulldozer bot merged commit b975e59 into master May 27, 2020
@soloio-bulldozer soloio-bulldozer bot deleted the common-api branch May 27, 2020 19:38
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

3 participants