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

Fixing Pointer-to-String Flags and Improve Testing #28

Merged
merged 1 commit into from
Jul 2, 2021

Conversation

otaviof
Copy link
Member

@otaviof otaviof commented Jun 25, 2021

Changes

Refactoring the flags using string-to-pointer inside a nested structure to avoid StringPointerValue. Additionally, covering the flags package with unit-tests, every command-line flag is checked.

Fixes #26

Testing

Execute the following commands in order, so a new BuildRun object is created using a service-account named "test":

# first, create a new build
shp build create nodejs-ex --source-url="https://github.com/otaviof/nodejs-ex.git" # ...

# then, create a new build-run using service-account name option
shp buildrun create br --buildref-name=nodejs-ex --sa-name=test

# and, assert service-account name was issued
oc get buildruns.shipwright.io br -o json |jq '.spec.serviceAccount'

The last command should confirm the desired service-account is in place:

$ oc get buildruns.shipwright.io br -o json |jq '.spec.serviceAccount'
{
  "name": "test"
}

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

Release Notes

- Fixing command-line flags based on pointer-to-string, for instance "--sa-name"
- Adding tests to prevent comment-line flag issues in the future

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none labels Jun 25, 2021
@openshift-ci openshift-ci bot requested review from alicerum and sbose78 June 25, 2021 08:12
@otaviof
Copy link
Member Author

otaviof commented Jun 25, 2021

/cc @coreydaley

@openshift-ci openshift-ci bot requested a review from coreydaley June 25, 2021 08:12
@otaviof otaviof force-pushed the issue-26 branch 2 times, most recently from 94d0b4d to 3233745 Compare June 25, 2021 10:41
@otaviof otaviof requested a review from adambkaplan June 25, 2021 16:20
@otaviof otaviof force-pushed the issue-26 branch 6 times, most recently from f93735f to 48d9c0d Compare June 28, 2021 07:58
@otaviof
Copy link
Member Author

otaviof commented Jun 28, 2021

/retitle Testing Flags

@openshift-ci openshift-ci bot changed the title [WIP] Testing Flags Package Testing Flags Jun 28, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2021
@otaviof otaviof added the kind/bug Categorizes issue or PR as related to a bug. label Jun 28, 2021
Covering the flags generate by this package with unit-testing, so every
command-line flag is checked. Also, adding unit-tests on
StrategyKindValue and StringPointerValue.
@gabemontero
Copy link
Member

if we are fixing an issue @otaviof there really should be a release note

@gabemontero
Copy link
Member

along with that, update the PR title to give some sort of hint that something is being fixed

@gabemontero
Copy link
Member

otherwise cool stuff :-) !!

/approve

I'll lgtm once the PR title / release note are addressed.

Or I can hold off if one of the other reviewers here chimes in that they want to take a pass at this before it merges

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 1, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero

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

The pull request process is described 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

@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note and removed release-note-none labels Jul 1, 2021
@otaviof
Copy link
Member Author

otaviof commented Jul 2, 2021

along with that, update the PR title to give some sort of hint that something is being fixed

Thanks, @gabemontero, indeed when fixing the behavior it should be mentioned on the title and release-notes. Please consider.

@otaviof
Copy link
Member Author

otaviof commented Jul 2, 2021

/retitle Fixing Pointer-to-String Flags and Improve Testing

@openshift-ci openshift-ci bot changed the title Testing Flags Fixing Pointer-to-String Flags and Improve Testing Jul 2, 2021
@gabemontero
Copy link
Member

thanks @otaviof

as I have not heard from anybody else on the team here wrt reviewing this soon, and it has been open for a week now, going to

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2021
@openshift-merge-robot openshift-merge-robot merged commit 2ca52a4 into shipwright-io:main Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command line flags for build and build run aren't propogated to the build or buildRun object
3 participants