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

CLI create project --manager creates a manager module #256

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

aadityasinha-dotcom
Copy link

Worked on #1410 to add project create --manager flag which will create a manager class.

Signed-off-by: aadityasinha-dotcom <[email protected]>
pkg/cmd/projectCreate.go Outdated Show resolved Hide resolved
pkg/cmd/projectCreate.go Outdated Show resolved Hide resolved
pkg/cmd/projectCreate.go Outdated Show resolved Hide resolved
pkg/cmd/projectCreate.go Outdated Show resolved Hide resolved
pkg/cmd/projectCreate.go Outdated Show resolved Hide resolved
pkg/cmd/projectCreate.go Outdated Show resolved Hide resolved
@techcobweb
Copy link
Contributor

Great start. A few other points...

  • When the manager project is generated, it probably needs to go into the list of child projects in the overall gradle file, and the parent pom.xml, so that it gets compiled.

  • When the manager name matches that of a feature, that should probably be dis-allowed, as the two things inhabit the same namespace.

  • The OBR will need to also include the manager project.

  • The generated manager needs to have some manager code inside of some sort. Perhaps defining an annotation which can cause an object created by the manager to be injected into a testcase.

  • A new testcase file needs generating in the test projects which uses the new manager, to show that the managers' annotation can be used at runtime.

Copy link
Contributor

@techcobweb techcobweb left a comment

Choose a reason for hiding this comment

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

See comments.

Signed-off-by: aadityasinha-dotcom <[email protected]>
@techcobweb
Copy link
Contributor

techcobweb commented Jul 15, 2024

Initial changes look good. But we still need to address these comments:

  • When the manager project is generated, it probably needs to go into the list of child projects in the overall gradle file, and the parent pom.xml, so that it gets compiled.
    For example:
// Creates a pom.xml file in the parent project folder for Maven projects.
func createParentFolderPom(
	...
) error {
        ...
        if err == nil {

		templateParameters := ParentPomParameters{
			Coordinates:      MavenCoordinates{ArtifactId: packageName, GroupId: packageName, Name: packageName},
			GalasaVersion:    galasaVersion,
			IsOBRRequired:    isOBRRequired,
			ObrName:          packageName + ".obr",
			ChildModuleNames: make([]string, len(featureNames)), << This will need to be one bigger when there is a manager module.
			IsDevelopment:    IsDevelopment,
		}
		// Populate the child module names
		for index, featureName := range featureNames {
			templateParameters.ChildModuleNames[index] = packageName + "." + featureName
		}

                // << Here you need to also add the manager name to the list.

		targetFile := utils.GeneratedFileDef{
			FileType:                 "pom",
			TargetFilePath:           packageName + "/pom.xml",
			EmbeddedTemplateFilePath: "templates/projectCreate/parent-project/pom.xml",
			TemplateParameters:       templateParameters}

		err = fileGenerator.CreateFile(targetFile, forceOverwrite, true)
	}
	return err

That would sort out the pom.xml

Similarly with the gradle file in the parent project:
createParentFolderSettingsGradle...

ChildModuleNames: make([]string, len(featureNames)), needs to be one bigger if the --manager module was used.

ie: When you generate a project with this setting, can you get it to compile and run using gradle or maven (or both) ?

  • When the manager name matches that of a feature, that should probably be dis-allowed, as the two things inhabit the same namespace.

    • As in when input parameters are validated.
  • The OBR will need to also include the manager project.

    • Similar to the above for the OBR pom.xml and gradle file.
  • The generated manager needs to have some manager code inside of some sort. Perhaps defining an annotation which can cause an object created by the manager to be injected into a testcase.

  • A new testcase file needs generating in the test projects which uses the new manager, to show that the managers' annotation can be used at runtime.

@techcobweb
Copy link
Contributor

If you want to talk about it, or want further help, pls get back to me.
Or if you want this PR re-reviewed pls add a comment here while tagging me perhaps ?
@techcobweb

@techcobweb
Copy link
Contributor

Are you aware that the unit tests now fail with your change ? do they fail for you ?

To build the code, use the ./build-locally.sh --delta syntax (assuming you are using a unix machine) ?

Also, you would do well to add more unit tests to verify that your code is doing what you think it it's doing. Everyone in my team uses test-driven-development (TDD), which means we build up a set of unit tests as a safety net to allow us to refactor the code without changing the behaviour later. Very useful. But we can't accept code which breaks them (or adapts them), and we don't like to take code which doesn't have unit tests specifically for the new function added. Also, we track our code coverage % which is running about 80%

Copy link
Contributor

@techcobweb techcobweb left a comment

Choose a reason for hiding this comment

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

Some comments in the code, and more comments in the PR to think about.

@techcobweb
Copy link
Contributor

Also, have you tried running this code ? Does it actually do what you expect ?

@aadityasinha-dotcom
Copy link
Author

Also, have you tried running this code ? Does it actually do what you expect ?

Yes it does work, I will send you the result on the slack

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.

2 participants