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

Change command for Lifecycle and Environment Path #3533

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

Conversation

bangelic
Copy link
Contributor

What changes are you introducing?

Updating command language.

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

The language in two different commands appears redundant.

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

Checklists

  • 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.13/Katello 4.15
  • Foreman 3.12/Katello 4.14 (Satellite 6.16)
  • Foreman 3.11/Katello 4.13 (orcharhino 6.11 on EL8 only; orcharhino 7.0 on EL8+EL9)
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9/6.10)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • We do not accept PRs for Foreman older than 3.7.

@bangelic
Copy link
Contributor Author

@sjha4 Can you take a look at the changes I made here and let me know if this is accurate to what was discussed with Jeremy? Thanks!

Copy link

github-actions bot commented Dec 18, 2024

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.

@bangelic bangelic force-pushed the bangelic-SAT-24004-Update-Lifecycle-Environment-documentation-for-creating-Lifecycle-Environment-path-using-CLI-procedure branch from 963a27a to cea9cc2 Compare December 19, 2024 18:46
Copy link
Contributor

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

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

Please, @bangelic, always add a link to the Jira issue in the PR description for better context of your changes.
https://issues.redhat.com/browse/SAT-24004

I think you're missing the point of the reported bug. The reporter wants to make the two commands more differentiated because it's not clear how and why they are different. Therefore, using the same --name example for both does not help.

We're creating an LC environment with each command, not the whole path. The LC env. path is a result of the chain of environments. Let's not use "path" in the --name.

Also, we don't have to use underscores in the example text when it's a name. Only labels use underscores. Let's not put them here.

@pr-processor pr-processor bot added the Waiting on contributor Requires an action from the author label Jan 8, 2025
@Lennonka
Copy link
Contributor

Lennonka commented Jan 8, 2025

Please, also fix the following in the last step. I cannot comment on it directly.
To view the chain of the lifecycle environment path

@sjha4
Copy link
Contributor

sjha4 commented Jan 8, 2025

For specifying path to add the environment after library, we exposed the path-id parameter in hammer in this PR:
Katello/katello#11268

Might be helpful to document this as well:
hammer-cli-katello]$ hammer lifecycle-environment create --label=test12 --name=test12 --prior-id=1 --organization-id=1 --path-id=13

@bangelic bangelic force-pushed the bangelic-SAT-24004-Update-Lifecycle-Environment-documentation-for-creating-Lifecycle-Environment-path-using-CLI-procedure branch from cea9cc2 to fbf1045 Compare January 8, 2025 21:16
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Jan 8, 2025
@Lennonka
Copy link
Contributor

Lennonka commented Jan 9, 2025

For specifying path to add the environment after library, we exposed the path-id parameter in hammer in this PR: Katello/katello#11268

Might be helpful to document this as well: hammer-cli-katello]$ hammer lifecycle-environment create --label=test12 --name=test12 --prior-id=1 --organization-id=1 --path-id=13

@sjha4 @bangelic The parameter should be documented in Hammer help. I don't think it's particularly helpful glueing this example command to this procedure. We do not document every Hammer parameter in product documentation as users can look it up in Hammer reference help. That's my two cents.

It's also out of the scope of this PR. We're fixing a bug here. :))

@bangelic bangelic force-pushed the bangelic-SAT-24004-Update-Lifecycle-Environment-documentation-for-creating-Lifecycle-Environment-path-using-CLI-procedure branch from fbf1045 to 8546a15 Compare January 10, 2025 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants