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

Use shell internal functionality instead of basename/dirname #87195

Closed

Conversation

@joakimr-axis
Copy link

joakimr-axis commented Jan 14, 2020

For generate-groups.sh and generate-internal-groups.sh existing built-in
shell variable handling can be used instead of calling external programs
basename and dirname. Also remove some superfluous quotes to improve
readability.

Signed-off-by: Joakim Roubert joakimr@axis.com

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


For generate-groups.sh and generate-internal-groups.sh existing built-in
shell variable handling can be used instead of calling external programs
basename and dirname. Also remove some superfluous quotes to improve
readability.

Change-Id: I42dd1f88754d589ed236ecc824c0f3ceb6fbbc7c
Signed-off-by: Joakim Roubert <joakimr@axis.com>
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 14, 2020

Hi @joakimr-axis. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 14, 2020

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: joakimr-axis
To complete the pull request process, please assign lavalamp
You can assign the PR to them by writing /assign @lavalamp in a comment when ready.

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

@joakimr-axis

This comment has been minimized.

Copy link
Author

joakimr-axis commented Jan 14, 2020

/assign @lavalamp

@joakimr-axis

This comment has been minimized.

Copy link
Author

joakimr-axis commented Jan 14, 2020

PR #84521 moved here.

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Jan 14, 2020

I would delete every shell file in the repo if I were allowed, so I may not be the best person to review this.

Using unquoted strings makes me very uncomfortable. Do we have a style guide we're following for shell? E.g. the internal google style guide wants those quotes to stay.

We use basename and dirname all over the place. Are you planning on changing all of our shell files?

I am far from a shell expert but I find the shell-native substitute to be super opaque. These files aren't performance critical (are they?) so I'd personally keep the more legible existing code.

@joakimr-axis

This comment has been minimized.

Copy link
Author

joakimr-axis commented Jan 15, 2020

Using unquoted strings makes me very uncomfortable.

In what parts of this patch are you uncomfortable? I can only see that it is those

[ "${GENS}" = all ]

parts, where you then would prefer the superfluous

[ "${GENS}" = 'all' ]

? (Using double quotes, "all", as in the present code is in all cases wrong since nothing is to be expanded). I can add those single quotes if you like.

We use basename and dirname all over the place. Are you planning on changing all of our shell files?

If it is requested, I could do that. No problem.
The reason I updated these two files was that I was working with this part of the code and ran across them.

I am far from a shell expert but I find the shell-native substitute to be super opaque. These files aren't performance critical (are they?) so I'd personally keep the more legible existing code.

I would not call my self a shell expert either, but those simple constructions are IMO sort of shell scripting 101 (and very well described in the shell man pages). Certainly it is much more performance efficient, just like not doing a couple of extra unneeded memcopies in C code, but more importantly using shell internals minimizes the external dependencies and thus add robustness.

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Jan 15, 2020

We may have different ideas of what is included in shell 101 :)

/assign @liggitt

Who should either have an answer or know who our current shell expert is :)

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 15, 2020

From my perspective, this change makes these scripts much less readable/maintainable. The difference in performance is not a concern here. I'll defer to @sttts on whether this change is desireable.

/cc @BenTheElder @cblecker
for shell feedback

/unassign

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Jan 15, 2020

I agree @liggitt, preferring:

  • quoting things as consistently as possible to encourage correct expansion behavior (including appropriate use of braces, avoid unintentional string splitting)
  • using basename is more appropriate, it is POSIX and clearer about the intention: path manipulation
@joakimr-axis

This comment has been minimized.

Copy link
Author

joakimr-axis commented Jan 15, 2020

Ok, I hear you. No worries, I will drop this PR.
Please do note, though, that the ##*/ construction is found in the present codebase in the files

vendor/github.com/client9/misspell/install-misspell.sh
hack/make-rules/verify.sh
hack/lib/swagger.sh
hack/lib/golang.sh
cluster/gce/util.sh
cluster/gce/upgrade-aliases.sh
cluster/gce/gci/configure.sh

so if not using ##*/ is the goal there is some maintenance pending.

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Jan 15, 2020

It's true, we have some fairly major maintenance ongoing... originally the scripts had little to no consistent standards, we've at least started incrementally requiring them to pass shellcheck but it's been a pretty huge effort to fix them all 😞

really appreciate anyone taking a stab at that, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.