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

new structure with smi and install commands #71

Closed
wants to merge 11 commits into from

Conversation

kumarabd
Copy link
Contributor

Signed-off-by: kumarabd abishekkumar92@gmail.com

Signed-off-by: kumarabd <abishekkumar92@gmail.com>
@leecalcote
Copy link
Member

@kumarabd
Copy link
Contributor Author

@mgfeller @kushthedude It would be great if we can discuss on the new structure and the principles that needs to be defined to follow them throughout the adaptor repositories.

@kumarabd kumarabd requested a review from p-null August 27, 2020 20:23
@@ -0,0 +1,86 @@
---
Copy link
Member

Choose a reason for hiding this comment

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

Can this be centrally defined and imported from the Learn Layer5 repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes right, let me craete a helm chart for this, so that we can version them too.

Signed-off-by: kumarabd <abishekkumar92@gmail.com>
@kumarabd kumarabd self-assigned this Aug 28, 2020
@@ -13,5 +13,5 @@ var (

// ErrViper is the error object for viper
func ErrViper(err error) error {
return errors.New("701", fmt.Sprintf("Viper initialization failed with error: %s", err.Error()))
Copy link
Member

Choose a reason for hiding this comment

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

These error codes need to be centrally defined, not per repo, assuming that's what's happening here.

@leecalcote
Copy link
Member

Thank you for today’s demo, @kumarabd. Before merging, will you attend to each of the failed build checks?

set -e

if ! type "kubectl" > /dev/null 2>&1; then
printf "ERROR\tgrep cannot be found\n"

Choose a reason for hiding this comment

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

kubectl cannot be found

cpu: "200m"
memory: 500Mi
restartPolicy: Always
serviceAccount: meshery

Choose a reason for hiding this comment

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

serviceAccount is deprecated I just learned from my IDE

set -e

if ! type "kubectl" > /dev/null 2>&1; then
printf "ERROR\tgrep cannot be found\n"

Choose a reason for hiding this comment

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

kubectl cannot be found

@mgfeller
Copy link

Hi @kumarabd, great demo today! I'm still familiarizing myself with the code, it looks good and I learn a lot :-)

I also ran the adapter locally (microk8s).

scripts/smi/deploy.sh Outdated Show resolved Hide resolved
@@ -24,24 +23,31 @@ var (

// operations holds the supported operations inside mesh
operations = map[string]interface{}{
installKuma: map[string]interface{}{
"type": "INSTALL",
InstallKuma: map[string]interface{}{
Copy link
Member

Choose a reason for hiding this comment

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

Do adapter operations belong in a defaults file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@leecalcote Its either inject the config, or have it remained as a default. Injecting would mean that u do it via the main.go file. Do we want to have another hop for this?

Copy link
Member

Choose a reason for hiding this comment

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

Renaming this file to supported_operations.go will probably suffice.

Copy link

Choose a reason for hiding this comment

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

out of curiosity, do we have unsupported operations?

Signed-off-by: kumarabd <abishekkumar92@gmail.com>
Signed-off-by: kumarabd <abishekkumar92@gmail.com>
Signed-off-by: kumarabd <abishekkumar92@gmail.com>
Signed-off-by: kumarabd <abishekkumar92@gmail.com>
Signed-off-by: kumarabd <abishekkumar92@gmail.com>
Signed-off-by: kumarabd <abishekkumar92@gmail.com>
Signed-off-by: kumarabd <abishekkumar92@gmail.com>
scripts/smi/deploy.sh Outdated Show resolved Hide resolved
@leecalcote
Copy link
Member

@kumarabd a few conflicts to resolve here - https://github.com/layer5io/meshery-kuma/pull/71/conflicts

@leecalcote
Copy link
Member

@kumarabd good to merge after resolving conflicts. Let's release v0.2.0 after merge.

@kumarabd
Copy link
Contributor Author

kumarabd commented Sep 1, 2020

@kumarabd good to merge after resolving conflicts. Let's release v0.2.0 after merge.

@leecalcote Lets move with this first - #75 so that @shivaylamba can move ahead with his task. We can merge this PR along with the our meshkit decision. Sounds good?

@leecalcote
Copy link
Member

Sounds real good, @kumarabd 👍

@kumarabd kumarabd closed this Sep 9, 2020
@kumarabd kumarabd deleted the kumarabd/feature/kuma branch September 9, 2020 19:53
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.

3 participants