Skip to content

e2e tests: cleanup: capitalize CONSTANTS#14834

Merged
openshift-ci[bot] merged 1 commit intocontainers:mainfrom
edsantiago:capitalize_constants
Jul 6, 2022
Merged

e2e tests: cleanup: capitalize CONSTANTS#14834
openshift-ci[bot] merged 1 commit intocontainers:mainfrom
edsantiago:capitalize_constants

Conversation

@edsantiago
Copy link
Copy Markdown
Member

@edsantiago edsantiago commented Jul 5, 2022

A number of standard image names were lower-case, leading to
confusion in code such as:

registry := podman(... , "-n", "registry", registry, ...)
^--- variable                              ^---- constant

Fix a number of those to be capitalized and with _IMAGE suffix:

registry := podman(...,                    REGISTRY_IMAGE

Signed-off-by: Ed Santiago santiago@redhat.com

None

@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Jul 5, 2022
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Jul 5, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago

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

The pull request process is described here

Details 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2022
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was not actually part of the constant-name change; it is manual refactoring

Comment on lines 233 to 234
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Look at all these wonderful examples of ultra-confusing code

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, Ed! That is really confusing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I left some untouched (see also below). I lean toward having those be UPCASE also, but those seem unambiguous enough, and at least they have one Capital.

@openshift-ci openshift-ci bot added release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jul 5, 2022
@edsantiago edsantiago force-pushed the capitalize_constants branch 2 times, most recently from 0190929 to 7fb9834 Compare July 5, 2022 21:23
A number of standard image names were lower-case, leading to
confusion in code such as:

    registry := podman(... , "-n", "registry", registry, ...)
    ^--- variable                              ^---- constant

Fix a number of those to be capitalized and with _IMAGE suffix:

    registry := podman(...,                    REGISTRY_IMAGE

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago edsantiago force-pushed the capitalize_constants branch from 7fb9834 to 4fd5fb9 Compare July 5, 2022 21:36
@mheon
Copy link
Copy Markdown
Member

mheon commented Jul 5, 2022

LGTM

Copy link
Copy Markdown
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm

Thank you so much, Ed! It frequently confused me but I never had the courage to fix it.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2022
@openshift-ci openshift-ci bot merged commit 53ea7c0 into containers:main Jul 6, 2022
@edsantiago edsantiago deleted the capitalize_constants branch July 6, 2022 11:01
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants