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.

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

HPCC-25946 Add environment variables helm chart setting to doc #16539

Merged
merged 3 commits into from Sep 23, 2022
Merged

HPCC-25946 Add environment variables helm chart setting to doc #16539

merged 3 commits into from Sep 23, 2022

Conversation

g-pan
Copy link
Member

@g-pan g-pan commented Sep 20, 2022

See page 47 compiled book, PDF available upon request.

Signed-off-by: g-pan greg.panagiotatos@lexisnexis.com

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Cloud-compatibility
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Smoketest:

  • Send notifications about my Pull Request position in Smoketest queue.
  • Test my draft Pull Request.

Testing:

Successful integration testing: http://10.224.20.18/view/Docs-gp/job/Doc-Build-04-GPRepo2

@g-pan
HPCC-25946 Add environment variables helm chart setting to doc
bfbbf28 
Signed-off-by: g-pan <greg.panagiotatos@lexisnexis.com>
@github-actions
Copy link

https://track.hpccsystems.com/browse/HPCC-25946
Jira updated

JamesDeFabia
JamesDeFabia requested changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@JamesDeFabia JamesDeFabia left a comment

Choose a reason for hiding this comment

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

Question / comment inline

docs/EN_US/ContainerizedHPCC/ContainerizedMods/ConfigureValues.xml Outdated
environment values are defined under the <emphasis>#env:</emphasis>
portion of the delivered values.yaml as illustrated below.</para>

<programlisting>#env:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these be uncommented in the example?

ghalliday
ghalliday requested changes Sep 21, 2022
View reviewed changes
docs/EN_US/ContainerizedHPCC/ContainerizedMods/ConfigureValues.xml Outdated
<title>Environment Values</title>

<para>You can define Environment Variables in a YAML file. The
environment values are defined under the <emphasis>#env:</emphasis>
Copy link
Member

Choose a reason for hiding this comment

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

env: rather than #env: (similar to jim's comment below)

Copy link
Member

Choose a reason for hiding this comment

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

Actually it should be global.env

docs/EN_US/ContainerizedHPCC/ContainerizedMods/ConfigureValues.xml Outdated
environment values are defined under the <emphasis>#env:</emphasis>
portion of the delivered values.yaml as illustrated below.</para>

<programlisting>#env:
Copy link
Member

Choose a reason for hiding this comment

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

should be

global:
  env:
  - name: SMTPserver 
  - value: mysmtpserver

You can also specify environment variables for each individual component. Has that been documented?

Copy link
Member Author

Choose a reason for hiding this comment

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

no. this is the first mention of env
will see to add it here

@g-pan
HPCC-25946 Add environment variables helm chart setting to doc
e510b26 
Signed-off-by: g-pan <greg.panagiotatos@lexisnexis.com>
JamesDeFabia
JamesDeFabia approved these changes Sep 21, 2022
View reviewed changes
Copy link
Contributor

@JamesDeFabia JamesDeFabia left a comment

Choose a reason for hiding this comment

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

Good from my POV.

ghalliday
ghalliday requested changes Sep 21, 2022
View reviewed changes
docs/EN_US/ContainerizedHPCC/ContainerizedMods/ConfigureValues.xml Outdated
<programlisting>global:
env:
- name: SMTPserver
- value: mysmtpserver</programlisting>
Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be a leading '-' before value, otherwise it will create a new list entry, and fail the schema validation.

docs/EN_US/ContainerizedHPCC/ContainerizedMods/ConfigureValues.xml Outdated

<para>You can define Environment Variables in a YAML file. The
environment values are defined under the
<emphasis>global.env:</emphasis> portion of the delivered values.yaml
Copy link
Member

Choose a reason for hiding this comment

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

probably better without the trailing :

docs/EN_US/ContainerizedHPCC/ContainerizedMods/ConfigureValues.xml Outdated
<para>You can define Environment Variables in a YAML file. The
environment values are defined under the
<emphasis>global.env:</emphasis> portion of the delivered values.yaml
as illustrated below.</para>
Copy link
Member

Choose a reason for hiding this comment

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

as a list of name value pairs, as illustrated below.

Otherwise you haven't really described the format.

docs/EN_US/ContainerizedHPCC/ContainerizedMods/ConfigureValues.xml Outdated
- name: SMTPserver
- value: mysmtpserver</programlisting>

<para>The global.env section of the delivered values file adds default
Copy link
Member

Choose a reason for hiding this comment

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

question: What does the phrase "delivered values file" mean?

Copy link
Member Author

@g-pan g-pan Sep 21, 2022 •
edited

Choose a reason for hiding this comment

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

referring to the default/stock https://github.com/hpcc-systems/HPCC-Platform/blob/master/helm/hpcc/values.yaml file,
is there a better way to refer to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

perhaps "the supplied values.yaml file" is better?

@g-pan
HPCC-25946 Add environment variables helm chart setting to doc
8d00c0c 
Signed-off-by: g-pan <greg.panagiotatos@lexisnexis.com>
JamesDeFabia
JamesDeFabia approved these changes Sep 22, 2022
View reviewed changes
Copy link
Contributor

@JamesDeFabia JamesDeFabia left a comment

Choose a reason for hiding this comment

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

Good from my POV

@richardkchapman richardkchapman merged commit 743aeae into hpcc-systems:candidate-8.10.x Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghalliday ghalliday ghalliday approved these changes

@JamesDeFabia JamesDeFabia JamesDeFabia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@g-pan @ghalliday @JamesDeFabia @richardkchapman