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

https://issues.redhat.com/browse/ACM-15508 - provide more info about extraLabels for SiteConfig #7243

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

amolnar-rh
Copy link
Collaborator

@amolnar-rh amolnar-rh commented Nov 18, 2024

No description provided.

@amolnar-rh amolnar-rh changed the title https://issues.redhat.com/browse/ACM-15508 - provide more info about … https://issues.redhat.com/browse/ACM-15508 - provide more info about extraLabels for SiteConfig Nov 18, 2024
Copy link
Contributor

@dockerymick dockerymick left a comment

Choose a reason for hiding this comment

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

This looks good to me !

@openshift-ci openshift-ci bot added the lgtm label Dec 3, 2024
Copy link
Contributor

@cwilkers cwilkers 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, I just had one typo comment and a small clarification

}
----
--
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what the dashes are for

Suggested change
--

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I was jumping between ACM and OCP PRs, I'm not even sure this works in the ACM build.

Btw, it groups together paragraphs/code snippets that would have different level of indentation without it, which would make it look like one is in the next step. I often have to use the dashes instead of the + signs when they just stop working. I find it's much more reliable than playing around with the +. I attached two pictures from my local preview, hopefully that demonstrates it better :)

  • Without dashes:
    Screenshot From 2024-12-05 14-30-07

  • With dashes:
    Screenshot From 2024-12-05 14-30-35

@amolnar-rh amolnar-rh force-pushed the ACM-15508 branch 2 times, most recently from 378b9a5 to c2596a5 Compare December 5, 2024 14:37
@sakhoury
Copy link

sakhoury commented Dec 5, 2024

/lgtm

Copy link
Contributor

@dockerymick dockerymick left a comment

Choose a reason for hiding this comment

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

Sorry I reviewed this yesterday but forgot to click Submit review

@amolnar-rh amolnar-rh force-pushed the ACM-15508 branch 2 times, most recently from d387fb4 to 635dbe3 Compare December 6, 2024 16:11
...
}
----
Copy link
Contributor

Choose a reason for hiding this comment

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

So I made a commit to remove the two dashes. After doing that, the alignment appears as this now
Screenshot 2024-12-06 at 12 48 06 PM

maybe we should add the dashes back

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added them back in, should look good now.

@openshift-ci openshift-ci bot added the lgtm label Dec 6, 2024
Copy link

openshift-ci bot commented Dec 6, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amolnar-rh, dockerymick, sakhoury

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

openshift-ci bot commented Dec 9, 2024

New changes are detected. LGTM label has been removed.

@amolnar-rh amolnar-rh merged commit 385a41a into 2.12_stage Dec 9, 2024
1 of 2 checks passed
*Note:* The additional annotations and labels are only applied to the resources that were rendered through the referenced templates.

View the following example application of extraAnnotations and extraLabels:
Copy link
Contributor

Choose a reason for hiding this comment

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

@amolnar-rh will you monospace the parameters here?

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