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

Adding the Provider #1

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Adding the Provider #1

merged 1 commit into from
Feb 20, 2024

Conversation

fabi200123
Copy link
Contributor

  • Adding the OCI External Provider
  • Update README.md

Copy link
Member

@gabriel-samfira gabriel-samfira left a comment

Choose a reason for hiding this comment

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

Just a few comments.

}

func (o *OciProvider) DeleteInstance(ctx context.Context, instanceID string) error {
return o.ociCli.DeleteInstance(ctx, instanceID)
Copy link
Member

Choose a reason for hiding this comment

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

Delete should work by name or ID. Is the name also the ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, added the functionality to work for both (same for GetInstance)

Copy link
Member

Choose a reason for hiding this comment

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

Generally speaking, it's wise to maintain architectural boundaries. The DeleteInstance() function from the client should deal only with removing instances. The name implies the purpose and the OCI API does not expect an instance name. The DeleteInstance() from the provider is known to take either a provider ID, or in the case of a failure, we send the instance name in a last ditch attempt at cleanup.

In future releases, we will definitely differentiate between the 2 in the DeleteInstance() function of the provider, but for now, the resolution of the name to ID should happen in this function, not in the one defined in the client.

provider/provider.go Outdated Show resolved Hide resolved
internal/util/util.go Outdated Show resolved Hide resolved
if err != nil {
return nil, fmt.Errorf("failed to generate userdata: %w", err)
}
wrapped := fmt.Sprintf("<powershell>%s</powershell>", udata)
Copy link
Member

Choose a reason for hiding this comment

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

Does this work on OCI? I was under the impression they were using cloudbase-init.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works, but it is not needed. It works without as well, updated the code.

internal/spec/spec.go Show resolved Hide resolved
}
}

func (r *RunnerSpec) SetUserData() error {
Copy link
Member

Choose a reason for hiding this comment

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

Whenever you have a Set() function that changes a value in a struct, it's a good idea to put it behind a mutex to prevent race conditions. In that case you'll also need a Get() function that also tries to lock the mutex.

func (r *RunnerSpec) SetUserData() error {
   r.mux.Lock()
  defer r.mux.Unlock()
  // rest of code goes here
}

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.

2 participants