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

Fix Shared RP Documentation #3812

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix Shared RP Documentation #3812

wants to merge 6 commits into from
+143 −121

Conversation

razo7
Copy link
Collaborator

@razo7 razo7 commented Sep 2, 2024

Which issue this PR addresses:

It fixes the shared RP documentation.

What this PR does / why we need it:

  • Fix small caveats for creating the env file (missing export, and consistent string declaration)
  • Fix indentation levels, spaces, and numbering.
  • Reorganize the documentation file by moving the non-required procedures to the end under a new section.

Test plan for issue:

Not required but the documentation will be used in a new planned automation

Is there any documentation that needs to be updated for this PR?

Yes

How do you know this will function as expected in production?

razo7
razo7 commented Sep 2, 2024
View reviewed changes
docs/prepare-a-shared-rp-development-environment.md Outdated
export DATABASE_NAME="\$AZURE_PREFIX"
export RESOURCEGROUP='$RESOURCEGROUP_PREFIX-\$LOCATION'
export PROXY_HOSTNAME='vm0.$PROXY_DOMAIN_NAME_LABEL.\$LOCATION.cloudapp.azure.com'
export DATABASE_NAME='\$AZURE_PREFIX'
export RP_MODE='development'
export PULL_SECRET='$PULL_SECRET'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am pretty confident that USER_PULL_SECRET is missing here, but IDK how to create/find it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does make secrets get you the pull secret?

Copy link
Contributor

Choose a reason for hiding this comment

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

The pull secret mentioned here only consists for tokens for prod and int svc acr and not to say other registries like redhat or quay, these can provided to cluster during install or at later stages, RP doesn't need them and not a blocker for cluster creation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does make secrets get you the pull secret?

It does, but I am concerned about the USER_PULL_SECRET which is missing in the docs and exists in practice when you run SECRET_SA_ACCOUNT_NAME=rharosecretsdev make secrets.

@razo7
Copy link
Collaborator Author

razo7 commented Sep 2, 2024

/azp run ci,

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

hlipsig
hlipsig reviewed Sep 10, 2024
View reviewed changes
Copy link
Contributor

@hlipsig hlipsig left a comment

Choose a reason for hiding this comment

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

I think these changes do look correct. I'll ask for one more person to confirm. Please see my comment on Pull Secret and let me know if you get that solved.

docs/prepare-a-shared-rp-development-environment.md Outdated
export DATABASE_NAME="\$AZURE_PREFIX"
export RESOURCEGROUP='$RESOURCEGROUP_PREFIX-\$LOCATION'
export PROXY_HOSTNAME='vm0.$PROXY_DOMAIN_NAME_LABEL.\$LOCATION.cloudapp.azure.com'
export DATABASE_NAME='\$AZURE_PREFIX'
export RP_MODE='development'
export PULL_SECRET='$PULL_SECRET'
Copy link
Contributor

Choose a reason for hiding this comment

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

Does make secrets get you the pull secret?

@razo7
Copy link
Collaborator Author

razo7 commented Sep 10, 2024

/azp run e2e

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

rhamitarora
rhamitarora previously approved these changes Sep 17, 2024
View reviewed changes
@razo7 razo7 dismissed stale reviews from rhamitarora and SrinivasAtmakuri via d1b3165 September 17, 2024 08:38
@razo7 razo7 requested a review from bitoku as a code owner September 17, 2024 08:38
@razo7
Copy link
Collaborator Author

razo7 commented Sep 17, 2024

/azp run ci,e2e

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@razo7
Copy link
Collaborator Author

razo7 commented Sep 17, 2024

/azp run e2e

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@razo7
Copy link
Collaborator Author

razo7 commented Sep 19, 2024

I have updated the PR according to your great tip from Slack on using single quotes.

what’s confusing me is the single quotes on the exported variable which includes escaped $ . From my understanding this basically means you creating a literal string $RESOURCEGROUP_PREFIX…. Am I missing something here?

I have addressed that in the last commit. Let me know if that's ok and if can we merge it now.
CC @jaitaiwan

jaitaiwan
jaitaiwan previously approved these changes Sep 23, 2024
View reviewed changes
Copy link
Contributor

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

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

Looks good to me. One small spelling error.

@razo7
Copy link
Collaborator Author

razo7 commented Sep 23, 2024

/azp run ci,e2e

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@github-actions github-actions bot added the needs-rebase branch needs a rebase label Oct 10, 2024
@github-actions GitHub Actions
Copy link

Please rebase pull request.

rhamitarora
rhamitarora previously approved these changes Jan 20, 2025
View reviewed changes
razo7 added 6 commits January 31, 2025 08:40
@razo7 @kimorris27
Add missing export word
4fcb83b 
PARENT_DOMAIN_RESOURCEGROUP was missing an export
@razo7 @kimorris27
Use single quotes
5b85fe1 
Stay consistent and all the env vars are set using single quotes instead of double
@razo7 @kimorris27
Move ADMIN_OBJECT_ID upper
3666129 
The value of ADMIN_OBJECT_ID is used twice before it was declared as env variable. Now it will be declared beforehand
@razo7 @kimorris27
Fix indentation leveles, spaces, and numbering
1585113 
Due to indentation levels the page was rendered wrongly
@razo7 @kimorris27
Reorganize shared rp doc
54c8ca4 
Move the non-required procedures at the end under a new seciton and connect the certs creation with the certs rotation
@razo7 @kimorris27
Use double quotes
Loading
Loading status checks…
64bc873 
Stay consistent and all the env vars are set using double quotes instead of single. When single quotes on the exported variable includes escaped $ then you can create a literal string ''
@github-actions github-actions bot removed the needs-rebase branch needs a rebase label Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hlipsig hlipsig

@tsatam tsatam

@kimorris27 kimorris27

@jaitaiwan jaitaiwan

@SrinivasAtmakuri SrinivasAtmakuri

@rhamitarora rhamitarora

@edisonLcardenas edisonLcardenas

@jewzaam jewzaam

@bennerv bennerv

@hawkowl hawkowl

@rogbas rogbas

@petrkotas petrkotas

@jharrington22 jharrington22

@cblecker cblecker

@cadenmarchese cadenmarchese

@UlrichSchlueter UlrichSchlueter

@SudoBrendan SudoBrendan

@yjst2012 yjst2012

@anshulvermapatel anshulvermapatel

@tiguelu tiguelu

@mociarain mociarain

@bitoku bitoku

@fahlmant fahlmant

@sankur-codes sankur-codes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@razo7 @jaitaiwan @hlipsig @tsatam @SrinivasAtmakuri @rhamitarora @kimorris27 @edisonLcardenas