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.

Sign up for GitHub

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

Jump to bottom

Messages to Azure Sql Firewall Rule #356

Merged

melonrush13 merged 13 commits into Azure:master from melonrush13:sqlfirewallmsg

Oct 16, 2019

Contributor

melonrush13 commented Oct 14, 2019 •

edited

Loading

Address #326

What this PR does / why we need it:
Adding messages to the AzureSqlFirewall controller

Special notes for your reviewer:
Create an azure sql firewall instance
Run k get azuresqlfirewallrule -o yaml to see current message status of instance.

Should see a message field like:

➜  azure-service-operator git:(sqlfirewallmsg) ✗ k get azuresqlfirewallrule -o yaml
apiVersion: v1
items:
- apiVersion: azure.microsoft.com/v1alpha1
  kind: AzureSqlFirewallRule
  metadata:
    creationTimestamp: "2019-10-14T21:25:37Z"
    finalizers:
    - azuresqlfirewallrule.finalizers.azure.com
    generation: 1
    name: sqlf-allowazuresvcaccess
    namespace: default
    resourceVersion: "2069869"
    selfLink: /apis/azure.microsoft.com/v1alpha1/namespaces/default/azuresqlfirewallrules/sqlf-allowazuresvcaccess
    uid: 2a03a1c0-eec9-11e9-b1dc-5a2bc381628e
  spec:
    endipaddress: 0.0.0.0
    resourcegroup: resourcegroup-azure-operators-rush
    server: sqlserver-sample-777
    startipaddress: 0.0.0.0
  status:
    message: AzureSqlFirewallrulesqlf-allowazuresvcaccess successfully provisioned
    provisioned: true
kind: List
metadata:
  resourceVersion: ""

If applicable:

this PR contains documentation
this PR contains tests

melonrush13 added 4 commits

October 14, 2019 13:08


          adding types

90fa99d


          msgs

5040a5e


          fixing merge conflicts with naming or azuresql

d6d3eee


          more name changes to azure sql

8ee8240

melonrush13 requested a review from WilliamMortlMicrosoft

October 14, 2019 21:39

WilliamMortlMicrosoft previously requested changes

View reviewed changes

Contributor

WilliamMortlMicrosoft left a comment

Requesting at least one change... make sure Messages that occur need logging statements are passed to the logging call... this ensures things are aligned. Also, I'm confused by new updateerr conditions you added. I may be 100% wrong, but if you can explain them to me, that would be great. :-) Thanks Mel (sorry for so many comments)

controllers/azuresqlfirewallrule_controller.go Outdated

               		if helpers.HasFinalizer(&instance, azureSQLFirewallRuleFinalizerName) {
               			if err := r.deleteExternal(&instance); err != nil {
               				log.Info("Delete AzureSqlFirewallRule failed with ", "error", err.Error())
+              				instance.Status.Message = fmt.Sprintf("Delete AzureSqlFirewallRule failed with %s", err.Error())

Contributor

WilliamMortlMicrosoft Oct 14, 2019

move this above the log.Info and then send this exact message to log.Info... this will ensure everything is unified :-)

Contributor Author

melonrush13 Oct 14, 2019

I think this is good practice for the future - I'm going to leave it as is for now since it is consistent across the other controllers. I think it would be good to handle these all together as a new task 👍

Contributor Author

melonrush13 Oct 15, 2019 •

edited

Loading

Decided to fix it @WilliamMortlMicrosoft :) Good idea! I think this looks a lot cleaner, too!! Except, I still have the log in the same spot. I didn't think that mattered!

controllers/azuresqlfirewallrule_controller.go Outdated

               	if !helpers.HasFinalizer(&instance, azureSQLFirewallRuleFinalizerName) {
               		if err := r.addFinalizer(&instance); err != nil {
               			log.Info("Adding AzureSqlFirewallRule finalizer failed with ", "error", err.Error())
+              			instance.Status.Message = fmt.Sprintf("Adding AzureSqlFirewallRule finalizer failed with error %s", err.Error())

Contributor

WilliamMortlMicrosoft Oct 14, 2019

(same as above) use with log.Info

controllers/azuresqlfirewallrule_controller.go Outdated

               			if azerr, ok := err.(*errhelp.AzureError); ok {
               				if helpers.ContainsString(catch, azerr.Type) {
               					log.Info("Got ignorable error", "type", azerr.Type)
+              					instance.Status.Message = fmt.Sprintf("Got ignorable error of type %s", azerr.Type)

Contributor

WilliamMortlMicrosoft Oct 14, 2019

(same as above) use with log.Info

controllers/azuresqlfirewallrule_controller.go Outdated

Comment on lines 123 to 124

		if updateerr := r.Status().Update(ctx, &instance); updateerr != nil {
		r.Recorder.Event(&instance, corev1.EventTypeWarning, "Failed", "Unable to update instance")

Contributor

WilliamMortlMicrosoft Oct 14, 2019

I'm confused by this new error trap... please double check... also, the message should reflect the warning, no? (I may be 100% wrong on this)

controllers/azuresqlfirewallrule_controller.go Outdated

Comment on lines 161 to 162

		if updateerr := r.Status().Update(ctx, instance); updateerr != nil {
		r.Recorder.Event(instance, corev1.EventTypeWarning, "Failed", "Unable to update instance")

Contributor

WilliamMortlMicrosoft Oct 14, 2019

same as earlier, confused by the error trap and why message isnt being set

Contributor Author

melonrush13 Oct 14, 2019

So this is needed because we need to set the message variable which is done through r.Status().

But the error check is if it is somehow unable to update that variable (which shouldn't happen). Have this across the controllers as well

controllers/azuresqlfirewallrule_controller.go Outdated

Comment on lines 180 to 181

		if updateerr := r.Status().Update(ctx, instance); updateerr != nil {
		r.Recorder.Event(instance, corev1.EventTypeWarning, "Failed", "Unable to update instance")

Contributor

WilliamMortlMicrosoft Oct 14, 2019

same as earlier, confused by the error trap and why message isnt being set

controllers/azuresqlfirewallrule_controller.go Outdated Show resolved Hide resolved

controllers/azuresqlfirewallrule_controller.go Outdated

Comment on lines 242 to 244

+              		if updateerr := r.Status().Update(ctx, instance); updateerr != nil {
+              			r.Recorder.Event(instance, corev1.EventTypeWarning, "Failed", "Unable to update instance")
+              		}

Contributor

WilliamMortlMicrosoft Oct 14, 2019

same as earlier, confused by the error trap and why message isnt being set

controllers/azuresqlfirewallrule_controller.go Outdated

Comment on lines 249 to 251

+              	if updateerr := r.Status().Update(ctx, instance); updateerr != nil {
+              		r.Recorder.Event(instance, corev1.EventTypeWarning, "Failed", "Unable to update instance")
+              	}

Contributor

WilliamMortlMicrosoft Oct 14, 2019

same as earlier, confused by the error trap and why message isnt being set

controllers/azuresqlfirewallrule_controller.go Outdated

Comment on lines 260 to 262

+              		if updateerr := r.Status().Update(context.Background(), instance); updateerr != nil {
+              			r.Recorder.Event(instance, corev1.EventTypeWarning, "Failed", "Unable to update instance")
+              		}

Contributor

WilliamMortlMicrosoft Oct 14, 2019

same as earlier, confused by the error trap and why message isnt being set

melonrush13 added 4 commits

October 14, 2019 16:02


          removing unneeded msg

7e36839


          removing unneeded msg

914f1de


          passing msg var

4a8ba7f


          Merge branch 'master' into sqlfirewallmsg

54a700e

This was referenced Oct 15, 2019

Additional AzureSqlServer controller messages #362

Merged

Task: Add messages across SqlFirewallRule Controller #326

Closed

frodopwns and others added 3 commits

October 15, 2019 14:03


          small refactor experiment

9ce6b94


          Merge pull request #2 from frodopwns/melmess-reduce

62dc9df

small refactor experiment


          Merge branch 'master' into sqlfirewallmsg

516e3d7

frodopwns approved these changes

View reviewed changes

Contributor

frodopwns left a comment

lgtm


          Merge branch 'master' into sqlfirewallmsg

c6a184d

melonrush13 dismissed WilliamMortlMicrosoft’s stale review

October 15, 2019 21:28

all good


          Merge branch 'master' into sqlfirewallmsg

d922f04

melonrush13 merged commit b740950 into Azure:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet