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 Message field to SqlServerStatus and messages to sqlserver controller #258

Merged
merged 26 commits into from
Oct 9, 2019
Merged

Conversation

melonrush13
Copy link
Contributor

@melonrush13 melonrush13 commented Oct 1, 2019

#246

What this PR does / why we need it:

  • Adding message field to SqlServerStatus
  • Adding messages throughout the sqlserver controller using instance.Status.Message
  • Fixing spelling errors in sqlserver controller logs

Special notes for your reviewer:
To Test:

  • In terminal window 1: Run make run
  • In terminal window 2: Create a new sqlserver instance using kubectl create -f config/samples/azure_v1_sqlserver.yaml
  • Run k get sqlserver $sqlservernamel -o yaml

Should see the following output, with a new message field in the bottom under status:

➜azure-service-operator git:(msgstatus) ✗ k get sqlserver bearsawesomesql -o yaml
apiVersion: azure.microsoft.com/v1
kind: SqlServer
metadata:
  creationTimestamp: "2019-10-01T19:58:31Z"
  finalizers:
  - sqlserver.finalizers.azure.com
  generation: 1
  name: bearsawesomesql
  namespace: default
  resourceVersion: "601382"
  selfLink: /apis/azure.microsoft.com/v1/namespaces/default/sqlservers/bearsawesomesql
  uid: d8104caf-e485-11e9-b1dc-5a2bc381628e
spec:
  allowazureserviceaccess: true
  location: westus
  resourcegroup: bearrg
status:
  message: Successfully Submitted to Azure
  provisioned: true
  state: Ready➜  azure-service-operator git:(msgstatus) ✗ k get sqlserver bearsawesomesql -o yaml
apiVersion: azure.microsoft.com/v1
kind: SqlServer
metadata:
  creationTimestamp: "2019-10-01T19:58:31Z"
  finalizers:
  - sqlserver.finalizers.azure.com
  generation: 1
  name: bearsawesomesql
  namespace: default
  resourceVersion: "601382"
  selfLink: /apis/azure.microsoft.com/v1/namespaces/default/sqlservers/bearsawesomesql
  uid: d8104caf-e485-11e9-b1dc-5a2bc381628e
spec:
  allowazureserviceaccess: true
  location: westus
  resourcegroup: bearrg
status:
  message: Successfully Submitted to Azure
  provisioned: true
  state: Ready

If applicable:

  • this PR contains documentation
  • this PR contains tests

controllers/sqlserver_controller.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jananivMS jananivMS left a comment

Choose a reason for hiding this comment

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

Looks good. Some changes requested.

controllers/sqlserver_controller.go Outdated Show resolved Hide resolved
controllers/sqlserver_controller.go Show resolved Hide resolved
controllers/sqlserver_controller.go Outdated Show resolved Hide resolved
controllers/sqlserver_controller.go Outdated Show resolved Hide resolved
@melonrush13
Copy link
Contributor Author

Fixes:

  • Added messages to the sqlserver_controller_finalizer
  • Fixed some message sentences (CreateOrUpdateSQLServer, Firewall)

Q's:

  • Should I continue to follow the if updaterr across the code, or switch all the if's to err?
  • Make manifest generated the same file as my manual edit. Planning to keep my manual edits unless someone suggests otherwise

Would love a re-review when you get a minute :) @jananivMS @WilliamMortlMicrosoft

@melonrush13
Copy link
Contributor Author

Added! @frodopwns

Co-Authored-By: Erin Corson <frodopwns@gmail.com>
Copy link
Contributor

@frodopwns frodopwns left a comment

Choose a reason for hiding this comment

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

LGTM

@frodopwns frodopwns self-requested a review October 8, 2019 00:46
@frodopwns
Copy link
Contributor

@melonrush13 the test output says this:

pkg/resourcemanager/sqlclient/sqlclient_godsk.go:257:24: GoSDKClient.GetServer redeclared in this block
	previous declaration at pkg/resourcemanager/sqlclient/sqlclient_godsk.go:126:6
Makefile:97: recipe for target 'vet' failed
make: *** [vet] Error 2

but I don't see that error when I check out your branch

@melonrush13
Copy link
Contributor Author

@melonrush13 the test output says this:

pkg/resourcemanager/sqlclient/sqlclient_godsk.go:257:24: GoSDKClient.GetServer redeclared in this block
	previous declaration at pkg/resourcemanager/sqlclient/sqlclient_godsk.go:126:6
Makefile:97: recipe for target 'vet' failed
make: *** [vet] Error 2

but I don't see that error when I check out your branch

just ran this morning, @frodopwns, and also didn't get that error the tests spit out. Still able to get messages on a k get sqlserver x without problems.

Copy link
Contributor

@jananivMS jananivMS left a comment

Choose a reason for hiding this comment

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

lg except for the comments

controllers/sqlserver_controller.go Show resolved Hide resolved
controllers/sqlserver_controller.go Show resolved Hide resolved
@jananivMS jananivMS self-requested a review October 9, 2019 18:05
@@ -355,6 +373,7 @@ func (r *SqlServerReconciler) GetOrPrepareSecret(instance *azurev1.SqlServer) (*

if err := r.Get(context.Background(), types.NamespacedName{Name: name, Namespace: instance.Namespace}, secret); err == nil {
r.Log.Info("secret already exists, pulling creds now")
instance.Status.Message = "Secret exists on instance"
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is hanglng without a Update. I would suggest just removing this. Rest looks good

Copy link
Contributor

@jananivMS jananivMS left a comment

Choose a reason for hiding this comment

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

Everything else looks good.

@jananivMS jananivMS self-requested a review October 9, 2019 19:42
Copy link
Contributor

@jananivMS jananivMS left a comment

Choose a reason for hiding this comment

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

Looks good!

@melonrush13 melonrush13 merged commit 54b5e26 into Azure:azure-sql Oct 9, 2019
Porges pushed a commit that referenced this pull request May 11, 2021
* Rename comment helper methods for clarity
* Use renamed comment helpers
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.

None yet

4 participants