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

Updating some SkyDNS templates to v1beta3 #8273

Merged
merged 1 commit into from May 28, 2015

Conversation

krousey
Copy link
Contributor

@krousey krousey commented May 14, 2015

@vishh vishh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2015
protocol: UDP
template:
metadata:
creationTimestamp: null
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Remove creationTimestamp here and in other places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@vishh
Copy link
Contributor

vishh commented May 14, 2015

Can we re-use the configs under cluster/addons/dns/? replicas and domain fields have to set to some concrete value in this case.

@krousey
Copy link
Contributor Author

krousey commented May 14, 2015

I don't know if we can our not. I know these are used by util.sh in the same directory and passed through a rendering function.

@vishh
Copy link
Contributor

vishh commented May 14, 2015

IIUC, the skydns configs under cluster/addons/dns can be used as-is except for the salt configs inside it.

@vmarmol vmarmol removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2015
@krousey
Copy link
Contributor Author

krousey commented May 18, 2015

Please take another look as it was failing to build because some analytics links were missing from some README files. Is this ok?

- \"/etcd\"
- \"-bind-addr=127.0.0.1\"
- \"-peer-bind-addr=127.0.0.1\"
capabilities: {}
Copy link
Member

Choose a reason for hiding this comment

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

Please remove fields containing default values. The conversion script doesn't do that currently. If the field wasn't specified previously, it probably doesn't need to be specified in v1beta3, either, especially if it's just empty/false/0.

- \"-bind-addr=127.0.0.1\"
- \"-peer-bind-addr=127.0.0.1\"
image: quay.io/coreos/etcd:latest
imagePullPolicy: Always
Copy link
Member

Choose a reason for hiding this comment

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

Why does this have pull policy Always? It didn't before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defaults were added in the conversion and this is the default. Removing it.

@bgrant0607 bgrant0607 mentioned this pull request May 28, 2015
24 tasks
@bgrant0607
Copy link
Member

Ping

cc @lhuard1A

- \"/etcd\"
- \"-bind-addr=127.0.0.1\"
- \"-peer-bind-addr=127.0.0.1\"
image: quay.io/coreos/etcd:latest
Copy link
Member

Choose a reason for hiding this comment

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

In general, it's clearer to put name, then image, then other fields.

@bgrant0607
Copy link
Member

LGTM

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2015
thockin added a commit that referenced this pull request May 28, 2015
Updating some SkyDNS templates to v1beta3
@thockin thockin merged commit e88249e into kubernetes:master May 28, 2015
@krousey krousey deleted the cluser_skydns branch August 21, 2015 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants