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

Create auto-generated objects.go file #178

Conversation

achernevskii
Copy link
Contributor

Issue/Feature description

  • Structs for WAPI objects should be auto-generated to simplify the go-client usage and development.

How it was fixed/implemented

  • ObjectType field of the IBObject is replaced with the ObjectType
    method for each child object.

  • Override ReturnFields method for each struct. By default, it will return
    a set of standard fields for each respective struct.

  • Removed Ipv4Addr/Ipv6Addr fields from the HostRecord object.

  • Updated existing object manager unit tests.

  • The IBObjectManager interface is not changed.

Tests

  • Created a E2E connector facade, that will remove all objects created by e2e test after the test is done.

  • Created a set of E2E tests to check crud operations on the generated object structs.

Reviewers

@skudriavtsev

* ObjectType field of the IBObject is replaced with the ObjectType
  method for each child object.

* Override ReturnFields method for each struct. By default, it will return
  a set of standard fields for each respective struct.

* Removed Ipv4Addr/Ipv6Addr fields from the HostRecord object.

* Updated existing object manager unit tests.

* The IBObjectManager interface is not changed.
* Created ConnectorFacadeE2E that will delete objects created by test,
  when the test is done.

* Created a set of e2e tests to check simple CRUD operations on the
  generated objects.go
* This way any change in the static part doesn't affect the generator
  workflow.
…fields

* Previously used time.Time didn't unmarshalled right. Replaced it with a
  UnixTime struct which encapsulates the time.Time.
* Parsed WAPI doc pages are used for documentation comments by the
  generator.
* Test checks that such methods as ReturnFields, or ObjectType return
  valid values.
* Now ConnectorFacadeE2E will use slice of string instead of map for the
  deleteSet. This change is made to ensure the deletion order while
  sweeping objects.

* Created a CRUD test for the Host Record Infoblox object.
* Updated all unit and end-to-end tests to use v2 of the ginkgo BDD test
  suite.
* Now each end-to-end test has a set of labels, that describes if it's RO
  only, or RW test. Also added test IDs for wapi_framework_tests, those IDs
  are equal to the IDs in the original qa-nios-api-tests suite.
* Ginkgo v2 supports table driven tests, so objects_generated_test.go were
  rewritten using the ginkgo DescribeTable functionality.
* Now ConnectorFacadeE2E is able to update objects.
* Tests mimic the qa-nios-api-tests.
* Applied some tweaks to objects_generated.go.

* Updated some tests with new assertions.

* Created utils package, which can be used as a set of infoblox-go-client
  helper functions.
* Since updatable boolean field could be updated from true to false, we
  need to  serialize false values, when updating some resource, instead of
  omit them.
* The tests listed all the ranges before, that could result in a wrong
  range object coming up first in the API server response.

* Added query params to the test, so that it gets the right range object.
Copy link
Contributor

@skudriavtsev skudriavtsev left a comment

Choose a reason for hiding this comment

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

The major comments:

  1. 'omitempty' attribute will be harmfull in some cases; for example, if 'comment' field was set and a user wants to make it empty, it will not be possible to do using the implementation from this PR. As we discussed, this may be solved using a pointer to field's value instead of the value itself in the structure.
  2. It is hard to maintain the code located in a single file, at least when two or more people work on different issues//features but the code is in the same file. We can avoid this splitting the implementation into several files, one for each object type, for example.

Minor comment. It would be better to put an empty line between a field's definition in a structure and a comment for the next field in this structure. Just to improve readability.

…ointers

* Added empty lines between field definitions in objects_generated.go for
  better readability.

* Changed updatable string and uint32 fields to pointers since they can be
  updated to default values ("" and 0 respectively). Converting those
  fields to pointers will make it possible for json lib to distinguish
  between default and absent value.
@achernevskii
Copy link
Contributor Author

Hello @skudriavtsev,

'omitempty' attribute will be harmfull in some cases;

I couldn't think of a scenario, in which omitempty attribute could be "harmful". However it restricts a library user from using default values (e.g. "" for string) as some field values. That's inconvenient, so I've changed all the updatable field's types to pointers. This way nil pointer will mean an absence of a value, and pointer to an empty string in case of a string will mean an empty string value.

It is hard to maintain the code located in a single file

This code is meant to be auto-generated. If somebody needs to change this file, this person should update the generator configuration first, and then use the generator to update the file. In this case, I don't think that it's gonna be hard to maintain the code, since there are no merge conflicts, when we have the generator which should always create the same code, if configuration isn't changed. Also it's much more easier to explain developers that they can't manually update one file, instead of articulating the same thought about the 100+ auto-generated files.

It would be better to put an empty line between a field's definition in a structure and a comment for the next field in this structure. Just to improve readability.

Added empty lines between field definitions.

@skudriavtsev
Copy link
Contributor

At least, 'comment' field is not fixed: updating with an empty comment leads to no update at all. At many objects.

Besides, 'extattrs' field is also marked as 'omitempty', and this leads to the situation when we try to update with an empty list of EAs (removing all EAs) but update does not happen. This behavior must be changed.

* Created e2e test case that checks if DNS View object's comment can be
  updated to empty string.
@achernevskii
Copy link
Contributor Author

At least, 'comment' field is not fixed: updating with an empty comment leads to no update at all. At many objects.

@skudriavtsev, could you provide the object with a problem comment field?

Tested it out on the View object, comment field is updatable when I specify the empty string. You can see the test case here: 819584b.

However you should know, that API will not return an empty string, when comment is set to empty string, even if you specify comment in _return_fields. This is a WAPI behaviour, that cannot and shouldn't be changed on the client's side:
image

@skudriavtsev
Copy link
Contributor

Sorry, it seems that I have not retrieved the latest version of the code from your repository :-( I will re-test.

* Updated generation logic for objects_generated.go. Now omitempty json
  tag is skipped for all extensible attribute fields.

* Created e2e test for IPv4 Network object, which checks if all EAs of
  this object can be removed through update operation.
@achernevskii
Copy link
Contributor Author

Besides, 'extattrs' field is also marked as 'omitempty', and this leads to the situation when we try to update with an empty list of EAs (removing all EAs) but update does not happen. This behavior must be changed.

Changed EA behavior in 0e3d852. Now one can remove all extensible attributes via update operation.

achernevskii and others added 2 commits January 26, 2023 22:06
…records

* Two new e2e tests validate if view field is updatable in HOST and CNAME
  record objects.

* Fixed a small bug in ConnectorFacadeE2E that in some specific cases led
  to e2e test panic.

// This is a list of IPv6 Addresses for the host.
Ipv6Addrs []HostRecordIpv6Addr `json:"ipv6addrs,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skudriavtsev what this fixes exactly?

achernevskii and others added 10 commits June 28, 2023 11:35
* After e2e testing of a client on a new env, a couple of
  assertion errors came up. Fixed those errors by changing
  the assertion rules.
* For environments with multiple resources search fields
  should be specified in some cases to get the needed
  resource.
* If view argument is empty in NewRecordCNAME, View field
  in RecordCNAME should be set to nil, to mimic old
  go-client behaviour.
* Added possibility to remove ip addresses from HostRecord
  object, by skipping omitempty for record:host_ipv4addr
  and record:host_ipv6addr WAPI types.

* Added check for empty view string in the NewHostRecord
  method.

* Added populateNilLists method for WapiRequestBuilder.
…_generation' into feature/e2e_testing_client_generation
@hemanthKa677 hemanthKa677 merged commit 072a0c8 into infobloxopen:master Aug 21, 2023
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.

3 participants