config: Fail if ClientID not provided by palash25 · Pull Request #197 · taskcluster/taskcluster-cli

palash25 · 2018-07-08T18:46:16Z

Fixes #191

palash25 · 2018-07-08T19:26:05Z

@djmitche I was using return errors.New() before but for that to pass I had to add an extra return at the end of the function that didn't look good.

I hope this approach is fine since it looks similar to the one we use while loading configuration. But I am not sure why the tests failed. I tried rebuilding it a few times but the logs didn't change.

My previous approach of using return errors.New() was passing the tests should I use that one?

palash25 · 2018-07-08T19:28:18Z

I had another question, I don't see any test files in the codebase so how is taskcluster running tests on my PR?

djmitche · 2018-07-08T21:53:04Z

Taskcluster runs based on .taskcluster.yml.

As for the failure, it looks like this is it:

=== RUN   TestCommandGeneration
Either Client ID or Access Token not set
exit status 1
FAIL	github.com/taskcluster/taskcluster-cli/apis	0.035s

so there's a test somewhere (TestCommandGeneration) that was getting away with setting only one of those options, and that's not allowed anymore.

Or, it's setting none of the options. Note that it's OK to run tc-cli without any credentials. The case we want to fix is when exactly one of the clientId and accessToken are supplied.

palash25 · 2018-07-09T10:59:22Z

@djmitche I looked at TestCommandGeneration and I have gone back to my previous solution of returning an error since this is the only instant where Setup is called https://github.com/taskcluster/taskcluster-cli/blob/master/apis/provider_test.go#L46 I am not sure if there is anyway for the tests to pass unless we return something.

Please review

djmitche

This change modifies Setup to return an error, but the code that calls Setup isn't modified -- so that error is ignored.

Instead of returning an error, there's an example of error-handling in this function already, when Load returns an error. I'd suggest copying that approach instead of returning an error. The alternative is to make both failure conditions return error and then handle it in main.go.

djmitche · 2018-07-09T14:19:07Z

 			"revisionTime": "2017-03-30T08:48:43Z"
 		},
 		{
-			"checksumSHA1": "DCjamhJdcLN963jm6/yFoDbJvT4=",


The checksums shouldn't have changed here. This is likely due to a bug in govendor or something like that, rather than your fault. But please revert changes to this file.

Sorry about that I actually ran the tests locally which made govender sync run and then I must have committed it while amending my changes.

One question since running tests changes vendor.json should we run the tests locally or add vendor.json to .gitignore

djmitche · 2018-07-09T14:26:47Z

 		}
+	} else {
+		return errors.New("Either ClientID or Access Token not set")
 	}


If we draw out the expected logic chart here:

ok1 !ok1

ok2 set up credentials error

!ok2 error leave credentials nil

It's OK for both the clientId and accessToken to be missing (!ok1 && !ok2). But as you've written it, that condition would be regarded as an error.

palash25 · 2018-07-09T14:41:11Z

I'll make the changes by tonight.

djmitche · 2018-07-09T14:43:00Z

Good question re govendor -- and one beyond my Go abilities. @petemoore can you help?

djmitche · 2018-07-09T14:43:32Z

Note that running make test does not change anything for me, so I wonder if this is an issue of golang version or govendor version (is govendor even versioned?)?

palash25 · 2018-07-09T14:50:33Z

Note that running make test does not change anything for me, so I wonder if this is an issue of golang version or govendor version (is govendor even versioned?)?

Well I didn't have a lot of packages installed locally before running make test (not even govendor) so it installed a few packages maybe that's the reason that the json file changed.

palash25 · 2018-07-09T14:51:24Z

Another question: Do we have some kind of coverage testing for this repo?

djmitche · 2018-07-09T15:01:49Z

I don't think coverage testing is set up. In my experience, that sort of automation for Go projects requires a lot of manual intervention. For example, we run gometalinter and it continually either has bugs or invents new lints, so most PRs for this repo fail lint checks through no fault of the PR author. That means a maintainer has to research the issue and either fix gometalinter or fix the new lint issues in the existing source code. So, in the absence of someone with the time to keep up with such issues, I'd rather not add coverage checks.

Fixes #191

palash25 · 2018-07-10T11:43:21Z

@djmitche please review

djmitche

Haha, it ended up being very short!

Thanks!

djmitche · 2018-07-10T17:40:05Z

Are there other tc-cli bugs you'd like to work on? Or something else?

palash25 · 2018-07-10T17:56:52Z

@djmitche thanks for the merge

I would love to work more I will be looking for some other beginner issues in this repo and the others. If you happen to raise a new beginner issue feel free to cc me

djmitche self-requested a review July 8, 2018 18:53

djmitche suggested changes Jul 9, 2018

View reviewed changes

config: Fail if ClientID not provided

1acd61a

Fixes #191

djmitche self-requested a review July 10, 2018 14:06

djmitche approved these changes Jul 10, 2018

View reviewed changes

djmitche merged commit 10174f7 into taskcluster:master Jul 10, 2018

palash25 deleted the patch-1 branch July 10, 2018 17:53

This was referenced Jul 10, 2018

Running make updates vendor.json #198

Closed

All commands should accept TASKCLUSTER_ROOT_URL #199

Closed

jhford mentioned this pull request Jul 13, 2018

Error messages should print on stderr, not stdout #200

Closed

Conversation

palash25 commented Jul 8, 2018

Uh oh!

palash25 commented Jul 8, 2018

Uh oh!

palash25 commented Jul 8, 2018

Uh oh!

djmitche commented Jul 8, 2018

Uh oh!

palash25 commented Jul 9, 2018

Uh oh!

djmitche left a comment

Choose a reason for hiding this comment

Uh oh!

djmitche Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

palash25 Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

djmitche Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

palash25 commented Jul 9, 2018

Uh oh!

djmitche commented Jul 9, 2018

Uh oh!

djmitche commented Jul 9, 2018

Uh oh!

palash25 commented Jul 9, 2018

Uh oh!

palash25 commented Jul 9, 2018

Uh oh!

djmitche commented Jul 9, 2018

Uh oh!

palash25 commented Jul 10, 2018

Uh oh!

djmitche left a comment

Choose a reason for hiding this comment

Uh oh!

djmitche commented Jul 10, 2018

Uh oh!

palash25 commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants