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

apis: add a Cluster object #5250

Merged
merged 1 commit into from
Dec 6, 2021
Merged

apis: add a Cluster object #5250

merged 1 commit into from
Dec 6, 2021

Conversation

nicks
Copy link
Member

@nicks nicks commented Dec 3, 2021

Hello @milas, @nicksieger,

Please review the following commits I made in branch nicks/clusterdef:

4150728 (2021-12-03 15:33:35 -0500)
apis: add a Cluster object

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

// cluster or an existing Docker Daemon (for Docker Compose).
type ClusterSpec struct {
// Connection spec for an existing cluster.
Connect *ClusterConnection `json:"connect,omitempty" protobuf:"bytes,1,opt,name=connect"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Total nit, but s/Connect/Connection here for consistency with object type?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

//
// If not specified, will read the DOCKER_HOST env or use the default docker
// host.
Host string `json:"host,omitempty" protobuf:"bytes,1,opt,name=host"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Not critical, but I'd add a couple examples of valid Docker connection strings here - "host" is ambiguous (Docker's fault, I realize), so it'd be helpful for anyone reading the docs to realize this is something like unix:///var/run/docker.sock or tcp://localhost:12345 and not just a hostname like docker.cluster.local

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Comment on lines 141 to 144
// On Kubernetes, this will correspond to the kubernetes.io/arch annotation.
//
// On Docker, this will be the Architecture of the Docker daemon.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to normalize between these or do they use the same values? 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

clarified that we're going to use GOARCH style!

Copy link
Member

Choose a reason for hiding this comment

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

Since arch comes from nodes in the cluster, we're all good until someone makes a multi-node, multi-arch cluster. Maybe note that fact/assumption of uniform, "cluster"-wide arch?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, didn't intend to suggest that this was the only arch, added a clarification

Copy link
Member

@nicksieger nicksieger left a comment

Choose a reason for hiding this comment

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

LGTM! Exciting to see.

@@ -0,0 +1,159 @@
/*
Copyright 2020 The Tilt Dev Authors
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Copyright 2020 The Tilt Dev Authors
Copyright 2021 The Tilt Dev Authors

old copyright template. .emacs aside: (add-hook 'write-file-hooks 'copyright-update) 😉

Comment on lines 141 to 144
// On Kubernetes, this will correspond to the kubernetes.io/arch annotation.
//
// On Docker, this will be the Architecture of the Docker daemon.
Copy link
Member

Choose a reason for hiding this comment

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

Since arch comes from nodes in the cluster, we're all good until someone makes a multi-node, multi-arch cluster. Maybe note that fact/assumption of uniform, "cluster"-wide arch?

@nicks nicks merged commit 96f2496 into master Dec 6, 2021
@nicks nicks deleted the nicks/clusterdef branch December 6, 2021 17:54
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