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

Upstreamize Hammer guide #3197

Merged
merged 19 commits into from
Aug 26, 2024
Merged

Conversation

Lennonka
Copy link
Contributor

@Lennonka Lennonka commented Aug 7, 2024

Offering the Hammer guide to upstream.

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.12

Copy link

github-actions bot commented Aug 7, 2024

The PR preview for 90dc266 is available at theforeman-foreman-documentation-preview-pr-3197.surge.sh

The following output files are affected by this PR:

show diff

show diff as HTML

@Lennonka Lennonka force-pushed the upstreamize-hammer-guide branch from 133a0a4 to e1d10e4 Compare August 12, 2024 20:27
@Lennonka
Copy link
Contributor Author

I've made an attempt to rewrite the standalone installation procedure for upstream.
And incorporated all suggestions.

@Lennonka Lennonka requested a review from ekohl August 12, 2024 20:40
@Lennonka Lennonka force-pushed the upstreamize-hammer-guide branch from 28133c5 to 5b2767f Compare August 16, 2024 04:01
@Lennonka Lennonka marked this pull request as ready for review August 16, 2024 04:25
@Lennonka
Copy link
Contributor Author

I've moved the assembly and modules where they belong. I suggest merging it as is and making further changes in other PRs.

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

  • I noticed that many modules start with a level 2 heading. I am OK with this for now and can change it later in a follow-up PR.
  • Unsure what "aqua-background" is for/ why "org" and "loc" has to be blue/yellow.
  • Unsure if we want to keep some rather long examples of stdout.

I had a glance at most changes but not all. I appreciate the effort to bring it upstream. All my suggestion can be applied by you or me after merging.

Merging now and improving later is perfectly fine for me. Any other concerns or approval?

guides/doc-Hammer_CLI/master.adoc Show resolved Hide resolved
guides/common/modules/con_hammer-cheat-sheet.adoc Outdated Show resolved Hide resolved
----
:log_level: 'debug'
----
include::proc_increasing-the-logging-level-for-hammer.adoc[]
Copy link
Contributor

Choose a reason for hiding this comment

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

something that I'd like to avoid down the road. OK for now because the file already has level 2 headings.

guides/common/modules/ref_orgs-locs-and-repos.adoc Outdated Show resolved Hide resolved
@Lennonka
Copy link
Contributor Author

Lennonka commented Aug 19, 2024

  • Unsure what "aqua-background" is for/ why "org" and "loc" has to be blue/yellow.

It's sort of a flag that is introduced and explained in General information. It can be done differently, but for now I'm keeping it as is.

@Lennonka Lennonka changed the title Upstreamize Hammer guide - raw Upstreamize Hammer guide Aug 19, 2024
Copy link
Contributor Author

@Lennonka Lennonka Aug 19, 2024

Choose a reason for hiding this comment

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

I'm not sure what to do with this file.

The problem is that the reference should be generated separately for Foreman, Katello, orcharhino, and Satellite because of branding and presence of Katello functionality in hammer help.
If I generate it in downstream and keep this file in upstream, our sync will overwrite it from upstream again. Therefore, it would be better to remove it in upstream.
However, if I don't include it in upstream, the build will not pass because it's required by master.adoc.

So the solution might be to exclude this file from syncing in downstream. Will you be able to do that in your sync too, @maximiliankolb?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I can handle this. No worries.

@maximiliankolb
Copy link
Contributor

@Lennonka Just to ensure that I understand the intended workflow:

  • for upstream Foreman and Foreman+Katello, we can check in the output ref_* files from hammer full-help --md > hammer_full_help.md && ./generate-hammer-reference.sh hammer_full_help.md
  • for downstream, we can do the same on Satellite/orcharhino and check in those files to downstream-only branches?

I ran this on orcharhino and got 95 files in total. Do you envision generating the references files on a minimal installation, with select plugins, or with all available plugins in Satellite/orcharhino?

@Lennonka
Copy link
Contributor Author

Lennonka commented Aug 20, 2024

  • for upstream Foreman and Foreman+Katello, we can check in the output ref_* files from hammer full-help --md > hammer_full_help.md && ./generate-hammer-reference.sh hammer_full_help.md
  • for downstream, we can do the same on Satellite/orcharhino and check in those files to downstream-only branches?

That is correct.

I ran this on orcharhino and got 95 files in total. Do you envision generating the references files on a minimal installation, with select plugins, or with all available plugins in Satellite/orcharhino?

In Satellite, we have most plugins enabled AFAIK, I would include all plugins. For Foreman and Foreman+Katello I would go for generating the full help on an instance with all Hammer plugins enabled, either with or without Katello plugin accordingly. I think it's better to include all plugins regardless of what the user has enabled. Better to have it and not use it than not have it when needed :)

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

One small request.

I had a look at the built docs without reading/testing everything. I think we can merge this as is and then improve later.

I will have a closer look at this as soon as I integrate this into downstream docs.

As there are no other comments, I say LGTM; please merge after a final tiny edit & green GHA.

Thanks again @Lennonka for this effort! 🚀

guides/common/modules/con_introduction-to-hammer.adoc Outdated Show resolved Hide resolved
@pr-processor pr-processor bot added the Waiting on contributor Requires an action from the author label Aug 21, 2024
Co-authored-by: Maximilian Kolb <[email protected]>
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Aug 21, 2024
@Lennonka Lennonka merged commit 094b37b into theforeman:master Aug 26, 2024
9 checks passed
@Lennonka Lennonka deleted the upstreamize-hammer-guide branch August 26, 2024 12:49
Lennonka added a commit that referenced this pull request Aug 26, 2024
Offering the Hammer CLI guide from Satellite to Foreman community
Co-authored-by: Maximilian Kolb <[email protected]>
@Lennonka
Copy link
Contributor Author

Cherry-picked:

@maximiliankolb maximiliankolb mentioned this pull request Sep 2, 2024
2 tasks
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.

4 participants