cleanup: keeping sdk scaffolds centralized#3349
cleanup: keeping sdk scaffolds centralized#3349camilamacedo86 merged 2 commits intooperator-framework:masterfrom camilamacedo86:just-go-cleanup
Conversation
estroz
left a comment
estroz
left a comment
There was a problem hiding this comment.
This approach could work. However the SDK controls what the Makefile template is for both upcoming Ansible and Helm plugins, so we can write these Makefile fragments directly into its template.
|
Hi @estroz,
Note that, we need to do the same for helm/go/ansible and then, by following your recommendation it means to duplicate the code in at least 6 different places/files which shows reduce the Maintenace ability and lead the code more susceptible to errors besides increase the effort when changes are required related to it. Then, see that these scaffolds are specific to SDK CLI and should not be pushed in the plugins repos Helm/Ansible for example. Let's imagine that we need to change the OLM subcommand and its scaffolds, if we follow your suggestion it means that in the future to achieve this goal we would need to do a PR against SDK, Ansible and Helm repos. Note that everything that is in the helm plugin will be extracted for its repo and there we will not have SDK or its specific features to be able to test/check it. So, the idea here is that these customizations should be centralized in the SDK CLI repo instead of it. Because of this, I'd be against to do that as I spoke with @joelanford which at the first moment also brings this idea. Is it make sense? |
|
HI @estroz @joelanford, All suggestions made were addressed with expectations that ones which go against a previous suggestion made its motivations. I cannot see anything else here that would blocks this PR get merged. I am looking for your approvals for we are able to merge it. |
|
|
||
| // Update the scaffolded Makefile with operator-sdk recipes. | ||
| // TODO: rewrite this when plugins phase 2 is implemented. | ||
| if err := initUpdateMakefile("Makefile"); err != nil { |
There was a problem hiding this comment.
Call p.run() here instead of at the end.
There was a problem hiding this comment.
It shows no make difference, however, done. 👍
| } | ||
|
|
||
| return nil | ||
| return p.run() |
There was a problem hiding this comment.
| return p.run() | |
| return nil |
| if operatorType == projutil.OperatorTypeGo { | ||
| makefileBytes = append(makefileBytes, []byte(makefileBundleFragmentGo)...) | ||
| } else { | ||
| // if is not go project then, has not the manifest target | ||
| makefileBytes = append(makefileBytes, []byte(makefileBundleFragmentNonGo)...) | ||
| } |
There was a problem hiding this comment.
| if operatorType == projutil.OperatorTypeGo { | |
| makefileBytes = append(makefileBytes, []byte(makefileBundleFragmentGo)...) | |
| } else { | |
| // if is not go project then, has not the manifest target | |
| makefileBytes = append(makefileBytes, []byte(makefileBundleFragmentNonGo)...) | |
| } | |
| switch operatorType { | |
| case projutil.OperatorTypeUnknown: | |
| return fmt.Errorf("unsupported plugin key %q", cfg.Layout) | |
| case projutil.OperatorTypeGo: | |
| makefileBytes = append(makefileBytes, []byte(makefileBundleFragmentGo)...) | |
| default: | |
| makefileBytes = append(makefileBytes, []byte(makefileBundleFragmentNonGo)...) | |
| } |
camilamacedo86
dismissed
joelanford’s stale review
dismissing for we are able to move forward, however, it was re-checked with @joe as well.
5 participants
Description of the change:
Just moving the SDK scaffolds for plugins to the sdk dir and allow it to be re-used.
Motivation for the change:
The SDK scaffolds will be required to other plugins. See for example #3295
Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments(seechangelog/fragments/00-template.yaml)website/content/en/docs