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

RHDEVDOCS-2668: Document top-level attribute definition in a devfile #161

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

gabriel-rh
Copy link
Contributor

@github-actions
Copy link

Click here to review and test in web IDE: Contribute

@github-actions
Copy link

github-actions bot commented Jul 21, 2022

🎊 Navigate the preview: https://devfile-docs-161.surge.sh 🎊

Copy link
Contributor

@elsony elsony left a comment

Choose a reason for hiding this comment

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

/lgtm

@@ -18,8 +21,30 @@ Use devfile attributes to configure various features and properties according to

.Procedure


. Define a custom attribute
. Define attributes in a `component`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
. Define attributes in a `component`
. Define the `attributes` in `components`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each entry in the components array is a single component - so I don't think it is correct to say

Define the attributes in components

I could remove the ticks so that it says "in a component" or "in components" - if we use ticks, then we should probably add the word "object" after component or components , like we do with Kubernetes/OpenShift resources.

I will add a colon at the end of the statement, as it has a follow-up.

uri: outerloop-deploy.yaml
----
====
. Define a custom attribute in `metadata`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
. Define a custom attribute in `metadata`
. Define a custom attribute in `metadata`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

Comment on lines +5 to +7
The `variables` object is a map of variable name-value pairs that are used for string replacement in the devfile. Variables are referenced using the syntax `+{{variable-name}}+` to insert the corresponding value in string fields in the devfile.

Variables can be defined at the top level of the devfile or in the `parent` object. String replacement with variables cannot be used for:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it help to rewrite this for the reader? For example:

As a <your role: developer, administrator>, you can define which variables devfiles replaces with strings. <the benefit of doing this>. etc...

Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping the passive voice here obscures who or what performs the action and whether the object is static or mutable. Or assumes that the reader already knows this information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the devfiles content has been shoehorned into procedures, when it should be reference content - we are documenting a syntax/schema, similar to an API.

Having stand-alone procedures telling a user how to add one line to a file, for example, saying "version: 2.2.0", is not a good use of procedures.

With reference content, I feel passive voice can be more appropriate at times.

Separately, the new docs platform for devfiles does not (yet) support modular content - so all those redundant assemblies that only contain a single include to a module will have to be replace with files containing the content itself.


.Procedure

. Add a variable definition at the top level in your devfile
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
. Add a variable definition at the top level in your devfile
. Add a variable definition at the top level in your devfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Colon added.

javaVersion: 11
...
----
. Reference the variable by name later in the devfile
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
. Reference the variable by name later in the devfile
. Reference the variable by name later in the devfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Colon added.

@@ -900,7 +900,7 @@ <h3 class="panel-title">Devfile schema - Version 2.2.0-alpha</h3>
<span class="json-property-required"></span>
</dt>
<dd>
<p>The port number should be unique.</p>
<p>Port number to be used within the container component. The same port cannot be used by two different container components.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this HTML file belong in the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The HTML is auto-generated by the build process and used in the docs.

I am unsure why it is causing whitespace differences, but I am assuming that any future PRs I make will not have this whitespace issue, so there won't be any diffs and then no corresponing commit.

I will check with Jake if I can just ignore these changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is generated output, it might make sense to add an entry to the .gitignore file so it doesn't show up in your PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is generated, but it is used by the docs build (like autogenerated API docs), so I don't think it can go in the .gitignore file as that would mean excluding it permanently.


[role="_abstract"]
Use devfile attributes to configure various features and properties according to user and tooling needs. Attributes are implementation-dependent and written in free-form YAML. The following devfile objects can have attributes:
Use devfile attributes to configure various features and properties according to user and tooling needs. Attributes are implementation-dependent and written in free-form YAML.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it help to name the user role in this sentence? For example, "As a developer, you can use..."

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

Signed-off-by: Gabriel McGoldrick <gmcgoldr@redhat.com>
…- writing review

Signed-off-by: Gabriel McGoldrick <gmcgoldr@redhat.com>
@openshift-ci
Copy link

openshift-ci bot commented Aug 8, 2022

New changes are detected. LGTM label has been removed.

@openshift-ci
Copy link

openshift-ci bot commented Aug 8, 2022

@rolfedh: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci
Copy link

openshift-ci bot commented Aug 8, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: elsony, gabriel-rh, rolfedh

The full list of commands accepted by this bot can be found 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

…- Vale issue

Signed-off-by: Gabriel McGoldrick <gmcgoldr@redhat.com>
@gabriel-rh gabriel-rh merged commit 820254d into devfile:master Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants