Skip to content

Conversation

a-hilaly
Copy link
Member

@a-hilaly a-hilaly commented Jul 6, 2021

Issue N/A

During code-generator early days, generate.Generator used to have
methods that generated controllers, apis and resources code. After few
refactoring iterations the code that outputs controllers Go code is
now situated in pkg/generate/ack, and generate.Generator only
contains methods to read/load CRDs and Type Definitions.

This PR contains two commits. The first one renames pkg/generate.Generator
to pkg/model.Model and the second moves generator unit tests to pkg/model
and pkg/generate/testdata to pkg/testdata

Description of changes:

  • Rename pkg/generate.Generator to pkg/model.Model, also renames
    all the variables and parameters that were used to represent a generator.
  • Move pkg/generate/testdata to pkg/testdata
  • Move all pkg/generate unit tests to pkg/model

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@a-hilaly
Copy link
Member Author

a-hilaly commented Jul 6, 2021

NOTE to reviews: please give this PR a manual merge. It will simplify the rebase of #121

Copy link
Contributor

@RedbackThomson RedbackThomson left a comment

Choose a reason for hiding this comment

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

Wondering why this refactor PR has a bunch of new files and methods in it. Also do you know if the generator is referenced in other documentation (such as in community) or in the Godocs, that might need changing? I understand doing a search all for "generator" could return a lot of false positives, but I worry that a change this large may introduce a fragmented understanding of the code.

@a-hilaly
Copy link
Member Author

a-hilaly commented Jul 7, 2021

@RedbackThomson This will not impact the existing godocs, since we are keeping the same methods for Inferrer/Generator - new methods will be added the pkg/model godoc and that's it.

@a-hilaly
Copy link
Member Author

a-hilaly commented Jul 7, 2021

/hold

reworking the refactor

@ack-bot ack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2021
a-hilaly added 2 commits July 7, 2021 17:07
This patch renames every `generate.Generator` mention to
`model.Model`, also all the variables and parameters that were
used to represent a generator (for example `g` for generator).
This patch moves all unit tests in `pkg/generate` (tests for the old
`generate.Generator`) to `pkg/model`.
Also moves all testdata to `pkg/testdata` and update
`testutil.NewModelForService` to properly build the test data path for
both `pkg/model` and `pkg/generate/code` uni tests
@a-hilaly
Copy link
Member Author

a-hilaly commented Jul 7, 2021

@RedbackThomson i pushed new refactoring commits conserving the CRD/TypeDef methods. PTAL

@RedbackThomson
Copy link
Contributor

New refactoring is that you've moved Generator inside the model package and it's now called Model? Is there more that I am missing?

@a-hilaly
Copy link
Member Author

a-hilaly commented Jul 7, 2021

New refactoring is that you've moved Generator inside the model package and it's now called Model? Is there more that I am missing?

Yes exactly, also moved generator unit tests under pkg/model/model_*_test.go, and pkg/generate/testdata to pkg/testdata

Copy link
Collaborator

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Yes, excellent.

Copy link
Contributor

@RedbackThomson RedbackThomson left a comment

Choose a reason for hiding this comment

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

Agree this makes a lot more sense. Great!

@RedbackThomson
Copy link
Contributor

/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented Jul 8, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes, RedbackThomson, vijtrip2

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [A-Hilaly,RedbackThomson,jaypipes,vijtrip2]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@RedbackThomson RedbackThomson merged commit d921da4 into aws-controllers-k8s:main Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants