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.

Sign up for GitHub

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

Jump to bottom

feat(plugin): add new user config generator #1342

Merged
merged 1 commit into from
Oct 4, 2023
Merged

feat(plugin): add new user config generator #1342

merged 1 commit into from
Oct 4, 2023

Conversation

byashimov
Copy link
Contributor

@byashimov byashimov commented Sep 6, 2023 •
edited
Loading

About this change—what it does

  • Adds user config generator for Terraform plugin framework
  • Migrates ServiceIntegration to the framework

@byashimov byashimov added the no changelog No changelog entries are required for this PR label Sep 6, 2023
@byashimov byashimov force-pushed the byashimov-new-uconfig-generator branch 12 times, most recently from bda0b12 to bec657b Compare September 13, 2023 19:04
@byashimov byashimov removed the no changelog No changelog entries are required for this PR label Sep 13, 2023
@byashimov byashimov force-pushed the byashimov-new-uconfig-generator branch from bec657b to 456b222 Compare September 13, 2023 19:10
@byashimov byashimov marked this pull request as ready for review September 14, 2023 07:33
@byashimov byashimov requested a review from a team September 14, 2023 07:33
Serpentiel
Serpentiel reviewed Sep 20, 2023
View reviewed changes
Copy link
Contributor

@Serpentiel Serpentiel left a comment

Choose a reason for hiding this comment

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

  • in all getter functions, drop get from the name, in Go it's a bad practice
  • drop DTO prefixes everywhere, in Go this is an anti-pattern

docs/resources/service_integration.md Show resolved Hide resolved
docs/resources/service_integration.md Show resolved Hide resolved
internal/plugin/service/serviceintegration/models.go Show resolved Hide resolved
internal/plugin/service/serviceintegration/models.go Show resolved Hide resolved
internal/plugin/service/serviceintegration/service_integration_data_source.go Show resolved Hide resolved
internal/plugin/service/serviceintegration/service_integration_resource.go Show resolved Hide resolved
internal/plugin/service/serviceintegration/userconfig.go Outdated Show resolved Hide resolved
internal/schemautil/plugin.go Show resolved Hide resolved
ucgenerator/main.go Show resolved Hide resolved
ucgenerator/models.go Show resolved Hide resolved
Serpentiel
Serpentiel suggested changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@Serpentiel Serpentiel left a comment

Choose a reason for hiding this comment

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

above review

@byashimov
Copy link
Contributor Author

  • in all getter functions, drop get from the name, in Go it's a bad practice
  • drop DTO prefixes everywhere, in Go this is an anti-pattern

There are structs for dto, terraform objects, getters, setters etc. With the same name. I need to distinguish them somehow.
The get prefix is convention for getter method, not function.

@byashimov byashimov force-pushed the byashimov-new-uconfig-generator branch from 456b222 to 8e37f31 Compare September 21, 2023 07:42
@Serpentiel
Copy link
Contributor

Serpentiel commented Sep 21, 2023 •
edited
Loading

There are structs for dto, terraform objects, getters, setters etc. With the same name. I need to distinguish them somehow.

tfoTables should be called this way instead: <name>ResourceModel or <name>DataSourceModel, Cf. organization_resource.go

also please make the name of all structs singular, plural naming is not a good practice

The get prefix is convention for getter method, not function.

how is this any different? please research online why get prefix is not used, it's for a reason, e.g. here are two good posts:

@ivan-savciuc
Copy link
Contributor

We should create v5 branch and merge it there instead of the master. We are still determining how long v5 will take to release; in this case, we are blocking the main branch.

@Serpentiel
Copy link
Contributor

We are still determining how long v5 will take to release

the changes that are proposed in this PR are safe to release with v4.x

@ivan-savciuc ivan-savciuc self-assigned this Sep 22, 2023
@byashimov byashimov force-pushed the byashimov-new-uconfig-generator branch 3 times, most recently from f595c86 to cf378f5 Compare September 22, 2023 13:14
@byashimov
Copy link
Contributor Author

byashimov commented Sep 22, 2023 •
edited
Loading

Rebased on master. CodeQL fails because of a known bug.

ivan-savciuc
ivan-savciuc suggested changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@ivan-savciuc ivan-savciuc left a comment

Choose a reason for hiding this comment

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

Tested and it works really well, I do have some minor comments thought

ucgenerator/main.go Show resolved Hide resolved
ucgenerator/main.go Show resolved Hide resolved
@byashimov byashimov force-pushed the byashimov-new-uconfig-generator branch 2 times, most recently from 5e2f5c3 to 97d6bef Compare September 25, 2023 08:32
@Serpentiel Serpentiel self-assigned this Sep 26, 2023
@Serpentiel Serpentiel added the enhancement New feature or request label Sep 26, 2023
@Serpentiel Serpentiel changed the title chore(plugin): add new user config generator feat(plugin): add new user config generator Sep 26, 2023
@byashimov byashimov force-pushed the byashimov-new-uconfig-generator branch from 97d6bef to 71ff402 Compare September 27, 2023 12:04
@byashimov
Copy link
Contributor Author

byashimov commented Sep 27, 2023 •
edited
Loading

Rebased on master, fixed conflicts, upgraded code to context client.

@byashimov byashimov force-pushed the byashimov-new-uconfig-generator branch 6 times, most recently from fda6275 to 319a51a Compare October 2, 2023 11:36
Serpentiel
Serpentiel approved these changes Oct 2, 2023
View reviewed changes
Copy link
Contributor

@Serpentiel Serpentiel left a comment

Choose a reason for hiding this comment

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

lgtm

@byashimov byashimov force-pushed the byashimov-new-uconfig-generator branch from 319a51a to 4595972 Compare October 4, 2023 09:46
@byashimov byashimov enabled auto-merge (squash) October 4, 2023 10:00
ivan-savciuc
ivan-savciuc approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@ivan-savciuc ivan-savciuc left a comment

Choose a reason for hiding this comment

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

LGTM

@byashimov byashimov merged commit e49fc63 into main Oct 4, 2023
10 checks passed
@byashimov byashimov deleted the byashimov-new-uconfig-generator branch October 4, 2023 11:05
Serpentiel pushed a commit that referenced this pull request Nov 6, 2023
@byashimov @Serpentiel
feat(plugin): add new user config generator (#1342)
3c8e896
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Serpentiel Serpentiel Serpentiel approved these changes

@ivan-savciuc ivan-savciuc ivan-savciuc approved these changes

Assignees

@ivan-savciuc ivan-savciuc

@Serpentiel Serpentiel

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@byashimov @Serpentiel @ivan-savciuc