-
Notifications
You must be signed in to change notification settings - Fork 0
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
refactor to support multiple providers #150
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jacobsalmela
force-pushed
the
modest-star-brand
branch
from
October 2, 2023 15:49
1eb04a2
to
7438eb1
Compare
jacobsalmela
force-pushed
the
modest-star-brand
branch
2 times, most recently
from
October 16, 2023 19:47
35d3bac
to
e015222
Compare
jacobsalmela
force-pushed
the
modest-star-brand
branch
2 times, most recently
from
October 30, 2023 20:18
c6e37f8
to
0948b7e
Compare
jacobsalmela
changed the title
begin refactor to support multiple providers
refactor to support multiple providers
Oct 30, 2023
jacobsalmela
force-pushed
the
modest-star-brand
branch
4 times, most recently
from
November 3, 2023 00:49
ecba98d
to
5d2ffab
Compare
Signed-off-by: Jacob Salmela <jacob.salmela@hpe.com>
Signed-off-by: Jacob Salmela <jacob.salmela@hpe.com>
Signed-off-by: Jacob Salmela <jacob.salmela@hpe.com>
Signed-off-by: Jacob Salmela <jacob.salmela@hpe.com>
Signed-off-by: Jacob Salmela <jacob.salmela@hpe.com>
Signed-off-by: Jacob Salmela <jacob.salmela@hpe.com>
jacobsalmela
force-pushed
the
modest-star-brand
branch
from
November 3, 2023 01:01
5d2ffab
to
8bd4ee8
Compare
Since I changed very little in the tests--only some verbiage and adding new fixtures--this can safely merge with your approval. There are still CSM-specific things to adjust and remove from cmd/domain packages, but this gives us a nice starting point. |
shunr-hpe
approved these changes
Nov 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary and Scope
Overview
cmd
,domain
, andprovider
packages and intocsm
cmd *cobra.Command, args []string
from thecmd
layer down each provider (allowing them to define the logic and flags)provider.ConfigOptions
since these can now be gathered direct from the cobra command or by reading the config filecsm
package calledToBeValidated
, as a result of the above (also to prevent import cycle errors)validate
functions to methods of*CSM
cani.yml
to be a map of providers and their respective optionsCsmOpts
type in thecsm
package, which holds all the cray thingsRisks and Mitigations
internal/provider/csm
NewSessionInitCommand()
function, which each provider will do to set their own options in their own packageConfigOptions{}
in favor ofcsm.CsmOpts{}
GetProviderOptions()
function for the upper layers to use (i.e. writing options to config file)ToBeValidated{}
object is created for anything needing validation (k8s CIDRS, roles/subroles, etc)Provider
interface belowinternal/provider
ConfigOptions
interface contstraintSlsProvider
type in favor of a provider constraintExportJson(ctx context.Context, datastore inventory.Datastore, skipValidation bool) ([]byte, error)
SetProviderOptions
constraint, which accepts a cmd and args (from cobra)GetProviderOptions
to get the options when neededinternal/inventory
NewDatastoreJSON()
and madeNewDatastoreJSONCSM()
for the csm providerinternal/domain
ExportJson()
function (not specific to sls)DomainOptions
intoDomain
since the provider can set these in their own package nowSetupDomain()
, which sets up the required three components for most ops: hardwaretypeLibrary, a datastore, and a provider interface objectExportSls()
since the provider has it as a method and is csm-specificcmd
setupDomain()
, which sets up the domain for every command to use. this is run by the root command as aPersistentPreRunE: setupDomain
, and it sets aD
variable (similar to thed
used by each package before)root.D
in places where an unexported var was used, this should always be the active domaininit()
. an example is theBootstrapInitCmd
is fairly generic, but when theNewSessionInitCmd()
from the provider is called, the cmd's flags and such are replaced by that set by the providersession init
has the biggest makeover since all the csm-specific info was moved into the csm package. it is adjusted to use theroot.D
objectDatastoreExists()
removed in favor of logic insetupDomain()
spec
Medium. A few tests needed modifications since we use a map of providers now instead of a singular one. The code is shuffled quite a bit, but I kept the test changes to a minimum for more confidence in the changes.