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

Use Renderer to generate the ConfigMap #2120

Merged
merged 6 commits into from
Dec 16, 2019

Conversation

slaperche-scality
Copy link
Contributor

Component:

buildchain

Context:

We want to generate SLS from the buildchain, and we have to shove huge string inside it while trying to keep it readable AND have a valid YAML in the end.

Summary:

Instead of creating/formatting the SLS content by hand, let's use a YAML renderer.

Acceptance criteria:

You can build a working ISO


Closes: #2070

@slaperche-scality slaperche-scality requested a review from a team as a code owner December 10, 2019 09:13
@bert-e
Copy link
Contributor

bert-e commented Dec 10, 2019

Hello slaperche-scality,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

@bert-e
Copy link
Contributor

bert-e commented Dec 10, 2019

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

@slaperche-scality slaperche-scality force-pushed the improvement/2070-use-renderer-for-config-map branch from a8f24cd to d276aa4 Compare December 10, 2019 15:21
NicolasT
NicolasT previously approved these changes Dec 13, 2019
Copy link
Contributor

@NicolasT NicolasT left a comment

Choose a reason for hiding this comment

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

LGTM. One minor concern: the SLS renderer takes a content object, whilst we often put multiple objects in a single SLS file as a YAML document stream (objects separated by ---). This renderer could be more generalized by taking a list of objects as objects and render them accordingly.

@slaperche-scality
Copy link
Contributor Author

LGTM. One minor concern: the SLS renderer takes a content object, whilst we often put multiple objects in a single SLS file as a YAML document stream (objects separated by ---). This renderer could be more generalized by taking a list of objects as objects and render them accordingly.

Good point, and shouldn't be too hard to support: I can add a new commit for that.

We will need it to implement a YAML renderer.

Refs: #2070
Signed-off-by: Sylvain Laperche <[email protected]>
Add a new serialization format: YAML.

This can be used as base for an SLS serializer.

In order to handle long string payload and binary content we add two
helpers (`YAMLDocument.text` and `YAMLDocument.bytestring`) that use
custom renderers in order to have a readable encoding (using `|` string
block literal) and right encoding (use Base64 for binary contents).

Refs: #2070
Signed-off-by: Sylvain Laperche <[email protected]>
PyYAML has type annotations provided by typeshed, but those are
incomplete.

The long term solution would be to improve the type annotations in
typeshed, but for now we will selectively ignore the errors.

Note that we can't simply ignore the whole YAML library (like we do for
`docker` or `doit`) because it *has* type annotations.
Trying to do so results in a mypy warning:

    note: unused 'type: ignore' comment

Refs: #2070
Signed-off-by: Sylvain Laperche <[email protected]>
For now we define a SLS as a YAML file with optional shebang and list of
imports.

Since both YAML and SLS renderer uses YAML dump, we extract it into an
helper.

Refs: #2070
Signed-off-by: Sylvain Laperche <[email protected]>
Use the brand new SaltState serializer to generate the Dex ConfigMap.
Get rid of the now useless helper that were used to manually generate
YAML-encoded strings.

Closes: #2070
Signed-off-by: Sylvain Laperche <[email protected]>
@slaperche-scality
Copy link
Contributor Author

Done, I've added support for list of objects.

Copy link
Contributor

@NicolasT NicolasT left a comment

Choose a reason for hiding this comment

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

Not 100% sure I'm 100% comfortable with this approach, since now we have an SLS in the Salt tree that's not represented by any file in the 'source' salt/ tree, but OK.

try:
dumper.open() # type: ignore
for document in data:
dumper.represent(document) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this then adds the ---?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've tested locally.

And the code is highly inspired from official PyYAML code.

@slaperche-scality
Copy link
Contributor Author

/approve

@bert-e
Copy link
Contributor

bert-e commented Dec 16, 2019

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

  • ✔️ development/2.5

The following branches will NOT be impacted:

  • development/1.0
  • development/1.1
  • development/1.2
  • development/1.3
  • development/2.0
  • development/2.1
  • development/2.2
  • development/2.3
  • development/2.4

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

  • Any commit you add on the source branch will trigger a new cycle after the
    current queue is merged.
  • Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

@bert-e
Copy link
Contributor

bert-e commented Dec 16, 2019

I have successfully merged the changeset of this pull request
into targetted development branches:

  • ✔️ development/2.5

The following branches have NOT changed:

  • development/1.0
  • development/1.1
  • development/1.2
  • development/1.3
  • development/2.0
  • development/2.1
  • development/2.2
  • development/2.3
  • development/2.4

Please check the status of the associated issue None.

Goodbye slaperche-scality.

@bert-e bert-e merged commit be59122 into development/2.5 Dec 16, 2019
@bert-e bert-e deleted the improvement/2070-use-renderer-for-config-map branch December 16, 2019 23:18
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.

3 participants