-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
/cc @coreydaley |
94d0b4d
to
3233745
Compare
f93735f
to
48d9c0d
Compare
/retitle Testing Flags |
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.
if we are fixing an issue @otaviof there really should be a release note |
along with that, update the PR title to give some sort of hint that something is being fixed |
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 |
[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 |
Thanks, @gabemontero, indeed when fixing the behavior it should be mentioned on the title and release-notes. Please consider. |
/retitle Fixing Pointer-to-String Flags and Improve Testing |
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 |
Changes
Refactoring the flags using string-to-pointer inside a nested structure to avoid
StringPointerValue
. Additionally, covering theflags
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":The last command should confirm the desired service-account is in place:
Submitter Checklist
Includes docs if changes are user-facingRelease Notes