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

local_path_provisioner: fix invalid podhelper yaml #10237

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

MrFreezeex
Copy link
Member

@MrFreezeex MrFreezeex commented Jun 20, 2023

What type of PR is this?
/kind bug

What this PR does / why we need it:
New line was not inserted between image and imagePullPolicy for some reasons with the jinja. Simplifying this altogether should fix this.
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
Should fix recent CI failure after the merge of #10054
Does this PR introduce a user-facing change?:

[local_path_provisioner] Fix invalid podhelper yaml

New line was not inserted between image and imagePullPolicy for some
reasons with the jinja. Simplifying this altogether should fix this.

Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 20, 2023
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 20, 2023
@MrFreezeex
Copy link
Member Author

MrFreezeex commented Jun 20, 2023

^ I cannot explain why the line was not inserted but that's what I got from reading the log of the pod in the CI:

time="2023-06-20T08:27:49Z" level=fatal msg="Error starting daemon: invalid YAMLToJSON the helper pod with helperPodYaml: apiVersion: v1\nkind: Pod\nmetadata:\n  name: helper-pod\nspec:\n  containers:\n  - name: helper-pod\n    image: busybox        imagePullPolicy: IfNotPresent"

So the \n before the last line is clearly missing hopefully this simplification of the jinja fixes it, but no idea why that happened in the first place... AFAIK you need {%- or -%} to influence those sort of things and this is not the case here 🤔

And this should have been caught before the merge of the PR I am also confused how we ended up here in the first place tbh :/

@0ekk
Copy link
Member

0ekk commented Jun 20, 2023

^ I cannot explain why the line was not inserted but that's what I got from reading the log of the pod in the CI:

time="2023-06-20T08:27:49Z" level=fatal msg="Error starting daemon: invalid YAMLToJSON the helper pod with helperPodYaml: apiVersion: v1\nkind: Pod\nmetadata:\n  name: helper-pod\nspec:\n  containers:\n  - name: helper-pod\n    image: busybox        imagePullPolicy: IfNotPresent"

So the \n before the last line is clearly missing hopefully this simplification of the jinja fixes it, but no idea why that happened in the first place... AFAIK you need {%- or -%} to influence those sort of things and this is not the case here thinking

And this should have been caught before the merge of the PR I am also confused how we ended up here in the first place tbh :/

Because the trim_blocks which the environment variable in Jinja2 is set to True by default on Ansible. https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/action/template.py#L59

Maybe it can be solved easily this way. https://github.com/ansible/ansible/blob/devel/test/integration/targets/template/tasks/main.yml#L166-L171
Or adding special header to template file like https://github.com/kubernetes-sigs/kubespray/blob/master/roles/kubernetes-apps/metallb/templates/pools.yaml.j2#L1

@MrFreezeex
Copy link
Member Author

MrFreezeex commented Jun 20, 2023

Hmmm I see so the {% if .. %} needed to be on their own line I guess, the version there works and is simpler anyway so that's fine here but thanks for the knowledge! :D

Copy link
Contributor

@mzaian mzaian 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 catching this @MrFreezeex

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MrFreezeex, mzaian

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

The pull request process is described here

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 Jun 20, 2023
@yankay
Copy link
Member

yankay commented Jun 21, 2023

Thanks @MrFreezeex

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2023
@k8s-ci-robot k8s-ci-robot merged commit 4ad89ef into kubernetes-sigs:master Jun 21, 2023
maciej-markowski pushed a commit to maciej-markowski/kubespray that referenced this pull request Jun 23, 2023
…0237)

New line was not inserted between image and imagePullPolicy for some
reasons with the jinja. Simplifying this altogether should fix this.

Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>
@yankay yankay mentioned this pull request Aug 24, 2023
pedro-peter pushed a commit to pedro-peter/kubespray that referenced this pull request May 8, 2024
…0237)

New line was not inserted between image and imagePullPolicy for some
reasons with the jinja. Simplifying this altogether should fix this.

Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants