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

Fix acc test for datasource of network #4

Merged
merged 2 commits into from
Apr 4, 2019
Merged

Fix acc test for datasource of network #4

merged 2 commits into from
Apr 4, 2019

Conversation

keiichi-hikita
Copy link
Contributor

In current implementation of network datasource test,
test is sometimes failed due to
existence of network which has same condition.
(Like status is ACTIVE as well, tenant_id is same, like that)

So this pull request changes followings.

  • remove status, tenant_id from argument of datasource
  • remove relevant tests to the above changes
  • remove them from argument section of corresponding document

In current implementation of network datasource test,
test is sometimes failed due to
existence of network which has same condition.
  (Like status is ACTIVE as well, tenant_id is same, like that)

So this pull request changes followings.

- remove status, tenant_id from argument of datasource
- remove relevant tests to the above changes
- remove them from argument section of corresponding document
@rintooo
Copy link
Contributor

rintooo commented Apr 3, 2019

LGTM. Confirmed acc tests passed.
To keep code clean, would you check below things, please?

  1. Should we delete line 81-84, 91-92 in ecl/data_source_ecl_network_network_v2.go?

    - var status string
    - if v, ok := d.GetOk("status"); ok {
    -    status = v.(string)
    - }
    
    - Status:      status,
    - TenantID:    d.Get("tenant_id").(string),
    
  2. It's better to add comment why this resource is necessary. Line 159 in ecl/data_source_ecl_network_network_v2.go.

    # write comment here
    resource "ecl_network_network_v2" "net2" {
    

- Add description for the test which uses plane as query condition.
- Remove needless logic from datasource.
@keiichi-hikita
Copy link
Contributor Author

Thanks for reviewing this.

  1. Should we delete line 81-84, 91-92 in ecl/data_source_ecl_network_network_v2.go?
    - var status string
    - if v, ok := d.GetOk("status"); ok {
    -    status = v.(string)
    - }
    
    - Status:      status,
    - TenantID:    d.Get("tenant_id").(string),
    

You are absolutely right.
I removed needless logic from datasource file.

  1. It's better to add comment why this resource is necessary. Line 159 in ecl/data_source_ecl_network_network_v2.go.
    # write comment here
    resource "ecl_network_network_v2" "net2" {
    

I think so too.
I added comment block above this test configuration function.

@rintooo rintooo merged commit dd87251 into nttcom:master Apr 4, 2019
@keiichi-hikita keiichi-hikita deleted the fix_network_datasource branch July 9, 2019 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants