-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fixing build issues and warnings #26603
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
Fixing build issues and warnings #26603
Conversation
_topic_map.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @sfortner-RH This is what I commented out per our discussion earlier
|
The preview will be available shortly at: |
modules/olm-terms.adoc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adellape You can't have more than one level 1 heading (= Heading) in a file, so this was my attempt at fixing this file. Can you take a look and see if this is okay how I added this, or if you have a different way/text you'd rather see?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that works. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahardin-rh Can you check this file? It had some incorrect levels and I just want a sanity check that they indentation/levels are still what you were expecting. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your changes LGTM! Thanks!
modules/olm-mirroring-catalog.adoc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adellape Can you check this file too please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks. A casualty of a recent un-wrap. 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, unfortunately gotta be careful about that happening!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adellape This too please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JStickler Can you please check this? This is fixing a build warning about being at the wrong level. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bergerhoffer, that's actually a serverless file, so calling @abrennan89 to make the call on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies @JStickler - thanks!
modules/nw-aws-nlb-new-cluster.adoc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jboxman Can you check this update please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your change aligns with the usage elsewhere in the install docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apinnick This PR is fixing build issues. Can you please check that this fix is correct? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this looks fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mburke5678 Can you check the changes in this file please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bergerhoffer I think I used === to make the text smaller. Seems to large as ==. Is there a better/proper way to do this? Maybe definition lists?
While you are in the file, could you male Syslog parameters: and Additional RFC5424 syslog parameters consistent? One has a colon and one doesn't. Probably remove the colon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mburke5678 Sure thing, removed the colon.
And yeah, you need to go in order of the heading level, you shouldn't use a lower level just because you want the font to be smaller.
If I had to make a suggestion here, I would say that these sections don't really belong in the procedure module, and should probably be reference modules included after the procedure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yeah, you need to go in order of the heading level, you shouldn't use a lower level just because you want the font to be smaller.
Makes sense. Not sure why I formatted it this way. Thanks for fixing!
|
@bergerhoffer thanks been thinking about these for weeks. |
8e0d623 to
478d03c
Compare
|
/lgtm Merging to get past build warnings and errors for l10n. |
|
/cherrypick enterprise-4.6 |
|
/cherrypick enterprise-4.5 |
|
@vikram-redhat: new pull request created: #26632 DetailsIn 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. |
|
@vikram-redhat: #26603 failed to apply on top of branch "enterprise-4.5": DetailsIn 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. |
|
@vikram-redhat: failed to push cherry-picked changes in GitHub: pushing failed, output: "To https://github.com/openshift-cherrypick-robot/openshift-docs\n ! [rejected] cherry-pick-26603-to-enterprise-4.6 -> cherry-pick-26603-to-enterprise-4.6 (non-fast-forward)\nerror: failed to push some refs to 'https://openshift-cherrypick-robot:[email protected]/openshift-cherrypick-robot/openshift-docs'\nhint: Updates were rejected because the tip of your current branch is behind\nhint: its remote counterpart. Integrate the remote changes (e.g.\nhint: 'git pull ...') before pushing again.\nhint: See the 'Note about fast-forwards' in 'git push --help' for details.\n", error: exit status 1 DetailsIn 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. |
Preview: https://fixin-build-issues--ocpdocs.netlify.app/openshift-enterprise/latest/welcome/index.html