-
Notifications
You must be signed in to change notification settings - Fork 98
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
Aci resource migration epg #1250
Aci resource migration epg #1250
Conversation
Work in progress to solve EPG part of #1042 |
e85c01b
to
f9a3794
Compare
f9a3794
to
6bf612d
Compare
3c72394
to
5175463
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1250 +/- ##
==========================================
+ Coverage 84.48% 84.69% +0.21%
==========================================
Files 75 91 +16
Lines 21956 33545 +11589
==========================================
+ Hits 18549 28412 +9863
- Misses 2415 3829 +1414
- Partials 992 1304 +312 ☔ View full report in Codecov by Sentry. |
9c83557
to
e503698
Compare
973839a
to
acfcaee
Compare
aff2314
to
cbefc5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cbefc5f
to
c97e512
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ipamEnabled: "no" | ||
ignores: | ||
- "apiMode" | ||
ignore_properties_in_test: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just have the following under ignore_properties_in_test?
ignore_properties_in_test:
- "ipamDhcpOverride"
- "ipamEnabled"
- "ipamGateway"
and..
Include the following in the test values like we usually do?
ipam_dhcp_override : "10.0.0.2"
ipam_enabled: "yes"
ipam_gateway: "10.0.0.1"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would do away with the additional complexity in the resource_example templates like having:
{{- else if .IgnoreInTest}}
We would only need .IgnoreInTest in testvars and the the code for ignoreTestProperty() in the generator will only return a bool value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I did like I did is because we use the test values for populating the examples. But since we exclude the values from testing because we do not have the correct version running we cannot do the lookup here. Let me know if you want me to spend time after my holiday to see how to do this differently but think it would require more refactor work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
3631de8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.