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

dkirillov/gh-26: Renderer and Executor determined by the shore config #73

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

dkirillov
Copy link
Collaborator

Overview

Github Issue: #26

Summary (required always)

This PR makes use of the new shore config, by loading it and using it to determine the type of renderer and executor to use.

(Functionality of LoadShoreConfig introduced in 7c38b57)
Done on a project that has a shore config, it will load it and use the types defined for the renderer and executor.
When a project does not have a shore config, it will drop a default one.

Notes

Unit tests pass

Screenshot 2023-04-05 at 3 55 24 PM

To test, installed a local version from branch's source

Screenshot 2023-04-05 at 3 08 08 PM

shore help requires no configuration and should always work, so it was tested.

shore help - outside a shore project - should work

Screenshot 2023-04-05 at 3 12 17 PM

shore help - inside a shore project - should work

Screenshot 2023-04-05 at 4 03 21 PM

shore render and shore save were picked and tested because render relies on the renderer type being set, and save relies on the renderer type and the executor type being set.

shore render - outside a shore project - shouldn't work

Screenshot 2023-04-05 at 3 11 16 PM

shore render - inside a shore project - should work - also drops a new shore config

Screenshot 2023-04-05 at 3 14 02 PM
Screenshot 2023-04-05 at 3 14 09 PM

This was the first time that this was ran in this shore project, so a shore config was created.

shore save - outside a shore project - shouldn't work

Screenshot 2023-04-05 at 3 11 39 PM

shore save - inside a shore project - should work - also loads an existing shore config

Screenshot 2023-04-05 at 3 15 50 PM

This was ran after shore render, and so the shore config was already there and was loaded and used.

shore render - inside a shore project, bad renderer type - shouldn't work

Screenshot 2023-04-05 at 3 19 58 PM

shore save - inside a shore project, bad executor type - shouldn't work

Screenshot 2023-04-05 at 3 19 19 PM

Checklist

  • My pull request title follows the format <username>/<gh-issue-#number>:<short-description>
  • My code passes existing unit tests
  • My code follows the code style set for the project
  • I have added at least one reviewer for this PR

@dkirillov dkirillov requested a review from a team as a code owner April 5, 2023 20:10
@dkirillov dkirillov changed the title dkirillov/gh-26: Rendered and Executor determined by the shore config dkirillov/gh-26: Renderer and Executor determined by the shore config Apr 5, 2023
@dkirillov dkirillov added enhancement New feature or request DevX Developer Experience related labels Apr 5, 2023
@dkirillov dkirillov self-assigned this Apr 5, 2023
Copy link
Contributor

@eyal-mor eyal-mor left a comment

Choose a reason for hiding this comment

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

Thank you very much for this last mile work 🙏

truly appreciated.

My questions/comments are mainly on styling and architectural design choices.
The UX was discussed multiple times and seems very solid.

Let's iterate on the comments and get this feature out!

Comment on lines 54 to 62
// Select the Renderer
switch strings.ToLower(shoreConfig.Renderer[`type`].(string)) {
case JSONNET:
d.Logger.Debug("Using the Jsonnet Renderer")
chosenRenderer = jsonnet.NewRenderer(d.Project.FS, d.Project.Log)
default:
return fmt.Errorf("the following Renderer is undefined: %s", shoreConfig.Renderer[`type`].(string))
}
d.Renderer = chosenRenderer
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this sections to it's own function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Looks a bit cleaner.

Comment on lines 64 to 72
// Select the Backend
switch strings.ToLower(shoreConfig.Executor[`type`].(string)) {
case SPINNAKER:
d.Logger.Debug("Using the Spinnaker Backend")
chosenBackend = spinnaker.NewClient(d.Project.Log)
default:
return fmt.Errorf("the following Executor is undefined: %s", shoreConfig.Executor[`type`].(string))
}
d.Backend = chosenBackend
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this section to it's own function (I.E. loadBackend).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Comment on lines +34 to +35
ShoreConfig config.ShoreConfig
ShoreConfigOpts config.ShoreConfigOpts
Copy link
Contributor

Choose a reason for hiding this comment

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

Aside from tests, where are these fields used?

Copy link
Collaborator Author

@dkirillov dkirillov Apr 17, 2023

Choose a reason for hiding this comment

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

Currently, they're not being used anywhere.

The intent is to have them there early so when render/save/execute/test-remote commands start making use of the shore config the information will already be there.

For instance, when the render command is ran it will have the shore config loaded in the dependencies as well as the chosen profile - and so it will be able to use the correct config based on the profile (all of which it will have access to).

@dkirillov dkirillov requested review from eyal-mor and a team April 17, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DevX Developer Experience related enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants