WIP: Add changes from dahendel & co #33

davidfischer-ch · 2020-03-28T09:40:18Z

Unfortunately the repository that Dustin Hendel is maintaining on GitLab is not sharing the same history. So I had to duplicate code with meld. I stripped the changes that were not relevant (such as disabling tests ...)

Colstuwjx:master and davidfischer-ch:master are entirely different commit histories.

Please can you review it, and adapt tests accordingly.
Unfortunately I don't have (yet) much experience in go.

Best Regards,

David

job_template.go

Colstuwjx · 2020-03-28T12:13:40Z

job_template.go

-	return result, nil
+	endpoint := "/api/v2/job_templates/" + strconv.Itoa(template.ID)
+	template.AllowCallbacks = true
+	template.HostConfigKey = uuid.NewV4().String()


Why we need to set these two by manually?

I do not know, the original commit : davidfischer-ch@72fdcdc.

CreateJobTemplateCallBack executes a PATCH HTTP Request to create the callback url and the generated host_config_key.

The values are set to get that result. OK?

job_template.go

Colstuwjx · 2020-03-28T12:32:01Z

organizations_test.go

+
+import (
+	"github.com/stretchr/testify/require"
+	"github.com/stretchr/testify/suite"


Introduce a new test dependency should think twice. I'm ok to this, but I think it should be another single PR for this change.

So I figured it out, the organization_test.go is special, this is an integration test powered by a test suite to make code DRY. However, its not enabled by default unless you specify the integration tag. See the latest commits.

Do you think its an idea to make a mock test instead?

I have a working AWX server, deployed with script from davidfischer-ch@d299f2a.

Yes, for the sake of "safe by default", if it's been disabled by default, and it also works if enabled, I'll be OK.

Indeed, since its an integration test it needs an actual AWX server, up and responding to http://127.0.0.1:80. Hence the script to provision it, using docker-compose.

Colstuwjx · 2020-03-28T12:36:49Z

Thanks for your PR!

I leave some comments for requesting changes. BTW, why do you disable the test cases teams_test.go and credentials_test.go?

davidfischer-ch · 2020-03-28T15:47:04Z

Hello,

I forgot to enable tests that were disabled when copying files from the contributors.

I think you have the right to modify the PR, I invite you to do the modifications because you are better in judging the changes.

Do you agree?

Co-Authored-By: Jacky Wu <Colstuwjx@gmail.com>

davidfischer-ch · 2020-03-28T16:28:20Z

Thanks for your PR!

I leave some comments for requesting changes. BTW, why do you disable the test cases teams_test.go and credentials_test.go?

I just enabled those tests.

davidfischer-ch · 2020-03-28T16:49:16Z

Some issues I have trying to test based on the steps in Travis Config:

david@ZenBookPro:~/develop/awx-go$ make lint
gometalinter: error: unknown linters: gosimple
Makefile:27: recipe for target 'lint' failed
make: *** [lint] Error 1

david@ZenBookPro:~/develop/awx-go$ ./codecov.sh
../../go/src/_/home/david/develop/awx-go/job_template.go:10:2: cannot find package "github.com/twinj/uuid" in any of:
	/home/david/go/src/_/home/david/develop/awx-go/vendor/github.com/twinj/uuid (vendor tree)
	/usr/lib/go-1.10/src/github.com/twinj/uuid (from $GOROOT)
	/home/david/go/src/github.com/twinj/uuid (from $GOPATH)

I suppose go.mod has to be updated? What is the best practice?
If you can take time to explain me how, I will be your Padawan apprentice.

Obviously same issue for make build, etc.

Colstuwjx · 2020-03-29T05:15:25Z

david@ZenBookPro:~/develop/awx-go$ make lint
gometalinter: error: unknown linters: gosimple
Makefile:27: recipe for target 'lint' failed
make: *** [lint] Error 1

The gometalinter repo has been archived, and I will make a PR to change to use golangci-lint, you could skip this part, since it's been called by travis CI right now.

I suppose go.mod has to be updated? What is the best practice?
If you can take time to explain me how, I will be your Padawan apprentice.

For this issue, yes, you need to update the go.mod as well.

Colstuwjx · 2020-03-29T05:42:00Z

It seems that you've NOT grant me write access to your branch, so I'd like to just leave the go mod fix code in this branch, see this commit.

What's more, you should also make the make test works, the struct def is also not good to me:

>> running all tests
# github.com/Colstuwjx/awx-go [github.com/Colstuwjx/awx-go.test]
./credentials_test.go:13:5: unknown field 'Type' in struct literal of type Credential
./credentials_test.go:14:5: unknown field 'URL' in struct literal of type Credential
./credentials_test.go:15:5: unknown field 'Related' in struct literal of type Credential
./credentials_test.go:26:5: unknown field 'SummaryFields' in struct literal of type Credential
./credentials_test.go:62:5: unknown field 'Created' in struct literal of type Credential
./credentials_test.go:66:5: unknown field 'Modified' in struct literal of type Credential
./credentials_test.go:72:5: unknown field 'Organization' in struct literal of type Credential
./credentials_test.go:73:5: unknown field 'CredentialType' in struct literal of type Credential
./credentials_test.go:74:5: unknown field 'Inputs' in struct literal of type Credential
./credentials_test.go:82:23: awx.CredentialService undefined (type *AWX has no field or method CredentialService)
./credentials_test.go:82:23: too many errors
FAIL	github.com/Colstuwjx/awx-go [build failed]
?   	github.com/Colstuwjx/awx-go/awxtesting/mockserver	[no test files]
?   	github.com/Colstuwjx/awx-go/awxtesting/mockserver/mockdata	[no test files]
FAIL
make: *** [test] Error 2

I suggest that align with the current model struct definitions.
Let me know if you need more helps, I will be glad to give a hand.
Thanks.

davidfischer-ch · 2020-03-30T07:33:44Z

I don't know exactly how you can contribute to the PR, however, I enabled this option since the beginning, in the meantime, I just merged your changes to my branch, thanks!

davidfischer-ch · 2020-03-30T13:14:49Z

@Colstuwjx this is a work in progress. I am enhancing tests and making many changes, I'll keep you informed in case I have questions or need your expertise.

Thank you.

Colstuwjx · 2020-03-30T13:19:18Z

I don't know exactly how you can contribute to the PR, however, I enabled this option since the beginning, in the meantime, I just merged your changes to my branch, thanks!

It maybe my mistake, I'll be here if you have any questions or need help.

@Colstuwjx this is a work in progress. I am enhancing tests and making many changes, I'll keep you informed in case I have questions or need your expertise.

Sure, welcome!

davidfischer-ch · 2020-03-30T15:16:19Z

Current status : All (enabled) tests pass.
However I have to add more mock data to enable remaining tests.

See davidfischer-ch@6049f8c.

I am wondering if you have some time to give a hand on this?

Thanks.

davidfischer-ch · 2020-03-30T15:17:49Z

Yet another remark: 6f91b03 is changing behavior, because the update methods are now using PATCH instead of PUT to allow partial updates. Of course full updates are still possible.

Colstuwjx · 2020-03-31T02:08:04Z

Yet another remark: 6f91b03 is changing behavior, because the update methods are now using PATCH instead of PUT to allow partial updates. Of course full updates are still possible.

For the change behavior, I suggest move them into another separate PR, we should NOT make this PR too robust, let's just focus on introduce more AWX resources and their tests in this PR.

Current status : All (enabled) tests pass.
However I have to add more mock data to enable remaining tests.

See davidfischer-ch/awx-go@6049f8c.

I am wondering if you have some time to give a hand on this?

Okay.

davidfischer-ch · 2020-03-31T06:49:42Z

Yet another remark: 6f91b03 is changing behavior, because the update methods are now using PATCH instead of PUT to allow partial updates. Of course full updates are still possible.

For the change behavior, I suggest move them into another separate PR, we should NOT make this PR too robust, let's just focus on introduce more AWX resources and their tests in this PR.

Current status : All (enabled) tests pass.
However I have to add more mock data to enable remaining tests.
See davidfischer-ch/awx-go@6049f8c.
I am wondering if you have some time to give a hand on this?

Okay.

Change behavior has now its own PR #34

Add changes from dahendel & co

801288b

Colstuwjx requested changes Mar 28, 2020

View reviewed changes

davidfischer-ch and others added 4 commits March 28, 2020 17:12

Follow Go import style

5c1e59f

Co-Authored-By: Jacky Wu <Colstuwjx@gmail.com>

Fix copy-pasting issue

01201b8

Apply suggestion from @Colstuwjx

1693877

Enable tests

370f65d

fix: fixed go mod and make build issue.

1dbe190

davidfischer-ch force-pushed the upstream branch from eec4778 to 6f91b03 Compare March 30, 2020 12:50

davidfischer-ch changed the title ~~Add changes from dahendel & co~~ WIP: Add changes from dahendel & co Mar 30, 2020

davidfischer-ch mentioned this pull request Mar 31, 2020

Favor partial update and always pass given params #34

Open

davidfischer-ch added 8 commits March 31, 2020 08:34

Favor partial update and always pass given params

23b7a78

Fix test suite

5f90946

Add script to run AWX using docker

4d36858

Fix tests, disable the ones missing mock data

5f15387

Require integration flag to run integration tests

713d9cb

Cleanup types

aaa09cc

Add make test-integration

f84c379

Fix typo

8cc3a8e

davidfischer-ch force-pushed the upstream branch from d21507f to 8cc3a8e Compare March 31, 2020 06:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add changes from dahendel & co #33

WIP: Add changes from dahendel & co #33

davidfischer-ch commented Mar 28, 2020 •

edited

Loading

Colstuwjx Mar 28, 2020

davidfischer-ch Mar 28, 2020

davidfischer-ch Mar 31, 2020

Colstuwjx Mar 28, 2020

davidfischer-ch Mar 30, 2020

davidfischer-ch Mar 31, 2020

Colstuwjx Mar 31, 2020

davidfischer-ch Mar 31, 2020 •

edited

Loading

Colstuwjx commented Mar 28, 2020

davidfischer-ch commented Mar 28, 2020

davidfischer-ch commented Mar 28, 2020

davidfischer-ch commented Mar 28, 2020 •

edited

Loading

Colstuwjx commented Mar 29, 2020

Colstuwjx commented Mar 29, 2020

davidfischer-ch commented Mar 30, 2020 •

edited

Loading

davidfischer-ch commented Mar 30, 2020

Colstuwjx commented Mar 30, 2020

davidfischer-ch commented Mar 30, 2020

davidfischer-ch commented Mar 30, 2020 •

edited

Loading

Colstuwjx commented Mar 31, 2020

davidfischer-ch commented Mar 31, 2020

WIP: Add changes from dahendel & co #33

Are you sure you want to change the base?

WIP: Add changes from dahendel & co #33

Conversation

davidfischer-ch commented Mar 28, 2020 • edited Loading

Colstuwjx Mar 28, 2020

Choose a reason for hiding this comment

davidfischer-ch Mar 28, 2020

Choose a reason for hiding this comment

davidfischer-ch Mar 31, 2020

Choose a reason for hiding this comment

Colstuwjx Mar 28, 2020

Choose a reason for hiding this comment

davidfischer-ch Mar 30, 2020

Choose a reason for hiding this comment

davidfischer-ch Mar 31, 2020

Choose a reason for hiding this comment

Colstuwjx Mar 31, 2020

Choose a reason for hiding this comment

davidfischer-ch Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

Colstuwjx commented Mar 28, 2020

davidfischer-ch commented Mar 28, 2020

davidfischer-ch commented Mar 28, 2020

davidfischer-ch commented Mar 28, 2020 • edited Loading

Colstuwjx commented Mar 29, 2020

Colstuwjx commented Mar 29, 2020

davidfischer-ch commented Mar 30, 2020 • edited Loading

davidfischer-ch commented Mar 30, 2020

Colstuwjx commented Mar 30, 2020

davidfischer-ch commented Mar 30, 2020

davidfischer-ch commented Mar 30, 2020 • edited Loading

Colstuwjx commented Mar 31, 2020

davidfischer-ch commented Mar 31, 2020

davidfischer-ch commented Mar 28, 2020 •

edited

Loading

davidfischer-ch Mar 31, 2020 •

edited

Loading

davidfischer-ch commented Mar 28, 2020 •

edited

Loading

davidfischer-ch commented Mar 30, 2020 •

edited

Loading

davidfischer-ch commented Mar 30, 2020 •

edited

Loading