Skip to content

Include Pod OS field #35985

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.

Sign up for GitHub

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

Jump to bottom
Merged
k8s-ci-robot merged 2 commits into from
Aug 22, 2022
Merged

Include Pod OS field #35985

k8s-ci-robot merged 2 commits into from
Aug 22, 2022

Conversation

@ravisantoshgudimetla
Copy link
Contributor

@ravisantoshgudimetla ravisantoshgudimetla commented Aug 15, 2022

Include Pod OS field in the pod concepts.

@netlify
Copy link

netlify bot commented Aug 15, 2022 •
edited
Loading

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit 84da6bb
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/6303ae35e26911000952cc2e

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 15, 2022
@ravisantoshgudimetla ravisantoshgudimetla mentioned this pull request Aug 15, 2022
Merged
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Aug 15, 2022
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Aug 15, 2022
@ravisantoshgudimetla
Copy link
Contributor Author

ravisantoshgudimetla commented Aug 15, 2022

cc @sftim

sftim
sftim reviewed Aug 15, 2022
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I have a few small suggestions that I hope make sense.

@sftim
Copy link
Contributor

sftim commented Aug 15, 2022

/milestone 1.25

Relevant to kubernetes/enhancements#2802

@k8s-ci-robot k8s-ci-robot added this to the 1.25 milestone Aug 15, 2022
@sftim sftim mentioned this pull request Aug 15, 2022
Closed
26 tasks
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 15, 2022
@ravisantoshgudimetla
Copy link
Contributor Author

ravisantoshgudimetla commented Aug 15, 2022

The changes you made are correct @sftim. Thank you :)

marosset
marosset reviewed Aug 15, 2022
View reviewed changes
content/en/docs/concepts/workloads/pods/_index.md

{{< feature-state state="stable" for_k8s_version="v1.25" >}}

You should set the `.spec.os.name` field to either `windows` or `linux` to indicate the OS on
Copy link
Contributor

@marosset marosset Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

nit:
Should this be 'To indicate the OS on which the pod is intended to run on, set the .spec.os.name field to either windows or linux. ?
This avoids language like should or can.

Copy link
Contributor

@sftim sftim Aug 16, 2022

Choose a reason for hiding this comment

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

I like that we're making a recommendation. It's more than “there's this field, you could use it if you like”; now the field is GA, it's “you ought to set this field”.

If we had the capacity it'd be nice to show a sample webhook that produces a Warning when you don't set that for a Pod / pod template.

Copy link
Contributor

@sftim sftim Aug 16, 2022

Choose a reason for hiding this comment

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

Given that there's System.Net.HttpListener, that sample webhook could even be written in PowerShell!

Copy link
Contributor Author

@ravisantoshgudimetla ravisantoshgudimetla Aug 16, 2022

Choose a reason for hiding this comment

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

If we had the capacity it'd be nice to show a sample webhook that produces a Warning when you don't set that for a Pod / pod template.

We can do that but it is left to the user as an exercise since the use-cases may differ.

@marosset
Copy link
Contributor

marosset commented Aug 15, 2022

LGTM (from sig-windows)

@jihoon-seo
Copy link
Member

jihoon-seo commented Aug 16, 2022

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 16, 2022
@ravisantoshgudimetla
Copy link
Contributor Author

ravisantoshgudimetla commented Aug 16, 2022

/retest

@ravisantoshgudimetla
Copy link
Contributor Author

ravisantoshgudimetla commented Aug 16, 2022

@sftim - Any other concerns here?

@bradtopol
Copy link
Contributor

bradtopol commented Aug 16, 2022

Assigning @sftim since he is already engaged on this.
/assign @sftim

@marosset
Copy link
Contributor

marosset commented Aug 17, 2022

@ravisantoshgudimetla can you take a look at the CI job failures?

@jihoon-seo
Copy link
Member

jihoon-seo commented Aug 19, 2022

@ravisantoshgudimetla to re-trigger Netlify build, you can (force-)push again.
For example, you might squash multiple commits into one single commit and then force-push.

@sftim
Copy link
Contributor

sftim commented Aug 19, 2022

I've triggered a new Netlify preview. No need to force-push a near identical commit in order to make that happen.

@ravisantoshgudimetla
Include Pod OS field
5206c7f 
Include Pod OS field in the pod concepts.
@reylejano
Copy link
Member

reylejano commented Aug 22, 2022

/approve

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: reylejano

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2022
@sftim
Copy link
Contributor

sftim commented Aug 22, 2022

/remove-label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot removed the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 22, 2022
sftim
sftim reviewed Aug 22, 2022
View reviewed changes
sftim
sftim reviewed Aug 22, 2022
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2022
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 22, 2022

LGTM label has been added.

DetailsGit tree hash: bf59892b3b9648959bb5132c5520d23bda7b16b4

@k8s-ci-robot k8s-ci-robot merged commit 51b647b into kubernetes:dev-1.25 Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@divya-mohan0209 divya-mohan0209 Awaiting requested review from divya-mohan0209

@tengqm tengqm Awaiting requested review from tengqm

2 more reviewers

@marosset marosset marosset left review comments

@sftim sftim sftim left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@sftim sftim

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

1.25

Development

Successfully merging this pull request may close these issues.

7 participants

@ravisantoshgudimetla @sftim @marosset @jihoon-seo @bradtopol @reylejano @k8s-ci-robot