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(api,cdsctl,sdk): add export worker model #3774

Merged
merged 7 commits into from
Dec 26, 2018
Merged

feat(api,cdsctl,sdk): add export worker model #3774

merged 7 commits into from
Dec 26, 2018

Conversation

bnjjj
Copy link
Contributor

@bnjjj bnjjj commented Dec 26, 2018

  1. Description
  2. Related issues
    close cdsctl worker model export #3767
  3. About tests
  4. Mentions

@ovh/cds

if err != nil {
return sdk.WrapError(err, "Unable to start tx")
}
defer tx.Rollback()

Choose a reason for hiding this comment

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

Error return value of tx.Rollback is not checked (from errcheck)

url += "?force=true"
}

mods := []RequestModifier{}

Choose a reason for hiding this comment

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

this value of mods is never used (from megacheck)

@@ -129,6 +129,49 @@ func (c *client) EnvironmentImport(projectKey string, content io.Reader, format
return messages, nil
}

// WorkerModelImport import a worker model via as code
func (c *client) WorkerModelImport(content io.Reader, format string, force bool) (*sdk.Model, error) {
var url string

Choose a reason for hiding this comment

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

should merge variable declaration with assignment on next line (from megacheck)

return fmt.Errorf("Error: VSphere main worker command empty")
}
}
break

Choose a reason for hiding this comment

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

redundant break statement (from megacheck)

}

//User must be admin of the group set in the model
var ok bool
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var ok bool
var isGroupAdmin bool

if g.ID == sdkWm.GroupID {
for _, a := range g.Admins {
if a.ID == u.ID {
ok = true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ok = true
isGroupAdmin = true

}

//User should have the right permission or be admin
if !u.Admin && !ok {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if !u.Admin && !ok {
if !u.Admin && !isGroupAdmin {

engine/api/worker/model_import.go Show resolved Hide resolved
}

if errAdd := InsertWorkerModel(db, &sdkWm); errAdd != nil {
if errPG, ok := errAdd.(*pq.Error); ok && errPG.Code == gorpmapping.ViolateUniqueKeyPGCode {
Copy link
Member

Choose a reason for hiding this comment

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

Use sdk.Cause(errAdd) then cast to *pq.Error (https://ovh.github.io/cds/contribute/error_management/). Is the error was wrapped this will to work.

engine/api/worker_model_import.go Outdated Show resolved Hide resolved
// IsValid returns error if worker model is invalid
func (wm *WorkerModel) IsValid() error {
if wm.Name == "" {
return fmt.Errorf("Error: worker model name is not provided")
Copy link
Member

Choose a reason for hiding this comment

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

All the error in this func should probably generates a bad request to the client, so can be sdk.NewErrorFrom(sdk.ErrWrongRequest, "Worker model name is not provided").

@richardlt richardlt merged commit 689b82c into master Dec 26, 2018
@bnjjj bnjjj deleted the feat_3767 branch January 3, 2019 09:11
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.

cdsctl worker model export
3 participants