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

test: fabric_service_profile resource/data_source acceptance tests Feature/cxf 83390 #559

Merged
merged 20 commits into from
Feb 13, 2024

Conversation

tutkat
Copy link
Contributor

@tutkat tutkat commented Feb 5, 2024

Added acceptance test for service profile

@tutkat tutkat requested a review from a team as a code owner February 5, 2024 18:25
@tutkat tutkat changed the title Feature/cxf 83390 Feature/cxf 83390 fix Feb 5, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 5, 2024

Codecov Report

Attention: 678 lines in your changes are missing coverage. Please review.

Comparison is base (a60c71f) 44.71% compared to head (1d862b8) 44.52%.
Report is 36 commits behind head on main.

Files Patch % Lines
equinix/resource_fabric_port.go 61.38% 139 Missing ⚠️
...ternal/resources/metal/project_ssh_key/resource.go 0.00% 131 Missing ⚠️
internal/resources/metal/ssh_key/resource.go 0.00% 130 Missing ⚠️
...rnal/resources/metal/project_ssh_key/datasource.go 0.00% 61 Missing ⚠️
...nternal/resources/metal/ssh_key/resource_schema.go 0.00% 45 Missing ⚠️
internal/resources/metal/vrf/resource.go 2.85% 34 Missing ⚠️
equinix/resource_fabric_connection.go 95.36% 26 Missing and 1 partial ⚠️
internal/resources/metal/project_ssh_key/models.go 0.00% 20 Missing ⚠️
internal/resources/metal/metal_connection/speed.go 0.00% 16 Missing ⚠️
equinix/fabric_mapping_helper.go 6.25% 15 Missing ⚠️
... and 10 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #559      +/-   ##
==========================================
- Coverage   44.71%   44.52%   -0.19%     
==========================================
  Files          98       98              
  Lines       19888    18866    -1022     
==========================================
- Hits         8893     8401     -492     
+ Misses      10961    10395     -566     
- Partials       34       70      +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@thogarty thogarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start, have requested changes to make. If you have questions add your comments to the PR for visibility.

equinix/data_source_fabric_service_profile_acc_test.go Outdated Show resolved Hide resolved
equinix/resource_fabric_service_profile_acc_test.go Outdated Show resolved Hide resolved
equinix/resource_fabric_service_profile_acc_test.go Outdated Show resolved Hide resolved
equinix/resource_fabric_service_profile_acc_test.go Outdated Show resolved Hide resolved
equinix/resource_fabric_service_profile_acc_test.go Outdated Show resolved Hide resolved
equinix/resource_fabric_service_profile_acc_test.go Outdated Show resolved Hide resolved
@tutkat tutkat changed the title Feature/cxf 83390 fix Feature/cxf 83390_tests Feb 6, 2024
@thogarty
Copy link
Contributor

thogarty commented Feb 7, 2024

@tutkat , the PR needs to have a prefix in the name that labels what the release type is. For this it is test:, please update. Additionally, this is a public/customer facing repo. The Jira ticket name is not a readable description of what changed. Please label this PR test: fabric_service_profile resource/data_source acceptance tests

No release type found in pull request title "Feature/cxf 83390_tests". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:

  • feat: A new feature
  • fix: A bug fix
  • docs: Documentation only changes
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • refactor: A code change that neither fixes a bug nor adds a feature
  • perf: A code change that improves performance
  • test: Adding missing tests or correcting existing tests
  • build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
  • ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
  • chore: Other changes that don't modify src or test files
  • revert: Reverts a previous commit

Copy link
Contributor

@thogarty thogarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More changes requested. Let me know if you have questions.

equinix/data_source_fabric_service_profile_acc_test.go Outdated Show resolved Hide resolved
equinix/resource_fabric_service_profile_acc_test.go Outdated Show resolved Hide resolved
equinix/resource_fabric_service_profile_acc_test.go Outdated Show resolved Hide resolved
@tutkat tutkat changed the title Feature/cxf 83390_tests test: fabric_service_profile resource/data_source acceptance tests Feature/cxf 83390 Feb 7, 2024
Copy link
Contributor

@thogarty thogarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested. Obvious bugs in PR. Please test after making changes to ensure correctness.

Additionally:

  1. Please add a description to the PR so that it's clear what the change is without reading the code
  2. Please update Names given to SPs. ds_con_sp_PFCR is unclear. Not sure what ds or con are referring two in either test sample.

equinix/data_source_fabric_service_profile_acc_test.go Outdated Show resolved Hide resolved
equinix/resource_fabric_service_profile_acc_test.go Outdated Show resolved Hide resolved
Signed-off-by: Tomasz Tutka <82565653+tutkat@users.noreply.github.com>
Signed-off-by: Tomasz Tutka <82565653+tutkat@users.noreply.github.com>
Signed-off-by: Tomasz Tutka <82565653+tutkat@users.noreply.github.com>
Copy link
Contributor

@thogarty thogarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested. Please ensure you build and run tests after making changes so that your code is ready for a merge.

equinix/resource_fabric_service_profile_acc_test.go Outdated Show resolved Hide resolved
equinix/resource_fabric_service_profile_acc_test.go Outdated Show resolved Hide resolved
@thogarty
Copy link
Contributor

Can confirm that service profile tests are working locally. Fabric GHA tests need some debugging and user data updates as well as sweeper changes before they will start passing again.

Approving change and merging.

@thogarty thogarty merged commit 6696918 into main Feb 13, 2024
6 of 9 checks passed
@thogarty thogarty deleted the feature/CXF-83390 branch February 13, 2024 01:02
@thogarty thogarty added the area/resources/fabric Issues related to Fabric and ECX APIs label Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/resources/fabric Issues related to Fabric and ECX APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants