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

feat: Implement the modulectl create command #49

Merged
merged 57 commits into from
Sep 25, 2024

Conversation

nesmabadr
Copy link
Contributor

…g file

Description

Changes proposed in this pull request:

  • Add create command implementation
  • Add unit tests

Related issue(s)
#28

@nesmabadr nesmabadr force-pushed the implement_create_module branch from 75ece5b to c2482f3 Compare September 9, 2024 14:04
@nesmabadr nesmabadr force-pushed the implement_create_module branch from c2482f3 to edfaf2b Compare September 9, 2024 14:29
@nesmabadr nesmabadr force-pushed the implement_create_module branch from 434354f to 334b96b Compare September 10, 2024 12:17
@kyma-bot kyma-bot added size/XXL and removed size/XL labels Sep 10, 2024
@nesmabadr nesmabadr force-pushed the implement_create_module branch 8 times, most recently from 6b1d2c2 to ed4cefb Compare September 11, 2024 10:34
@nesmabadr nesmabadr force-pushed the implement_create_module branch from ed4cefb to ff3fa58 Compare September 11, 2024 10:46
@lindnerby
Copy link
Member

/cla

@kyma-bot
Copy link
Contributor

Successfully reached out to cla-assistant.io to initialize recheck of PR #49

@nesmabadr nesmabadr force-pushed the implement_create_module branch from 3487d22 to a541a62 Compare September 24, 2024 14:09
Copy link
Contributor

@c-pius c-pius left a comment

Choose a reason for hiding this comment

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

Pls ignore comments for now. Just for myself as reminders so far.

internal/common/validation/validation.go Outdated Show resolved Hide resolved
internal/service/componentarchive/componentarchive.go Outdated Show resolved Hide resolved
internal/service/componentdescriptor/gitsources.go Outdated Show resolved Hide resolved
return resources, nil
}

func CreateCredMatchLabels(registryCredSelector string) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we considered making it a private func and test it from the same package?

internal/service/crdparser/crdparser.go Outdated Show resolved Hide resolved
Comment on lines +71 to +84
for {
var res Resource
err := decoder.Decode(&res)
if err != nil {
if err.Error() == "EOF" {
break
}
return "", fmt.Errorf("failed to parse YAML document: %w", err)
}

if res.Kind == "CustomResourceDefinition" && res.Spec.Group == group && res.Spec.Names.Kind == kind {
return res.Spec.Scope, nil
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this for loop. Does decoder.Decode actually change res?

internal/service/create/create.go Outdated Show resolved Hide resolved
internal/service/create/create.go Outdated Show resolved Hide resolved
@@ -1,20 +1,206 @@
package create

type ModuleConfigService interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we only at 27% coverage?

Comment on lines +22 to +24
if s.latestCommit != "" {
return s.latestCommit, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my understanding, what is the reason for "caching" the latest commit? Also considering the other GetRemoteFitFileContent method where we pass it as argument and not reading it from the property?

return defaultCRPath, nil
}

path := defaultCRPath
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be clearer if path is only declared within the if blocks, and at line 199 we return defaultCRPath given that none of the if branches executed.

internal/service/registry/registry.go Outdated Show resolved Hide resolved
internal/service/registry/registry.go Outdated Show resolved Hide resolved
internal/service/templategenerator/templategenerator.go Outdated Show resolved Hide resolved
tools/filesystem/archivefilesystem.go Outdated Show resolved Hide resolved
mmitoraj
mmitoraj previously approved these changes Sep 25, 2024
@kyma-bot kyma-bot merged commit 2a58e8e into kyma-project:main Sep 25, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the modulectl create module logic
6 participants