Fixing magic strings on operator flags #3327

azoppiserpa · 2023-12-15T01:31:26Z

Which issue this PR addresses:

There isn't a work item to be referenced for this change. The issue was found searching for // TODO in the code. It's a small refactor.

What this PR does / why we need it:

Fixes magic strings related to operator flags, and places them in a new flags.go file that can be reused instead of us having a bunch of magic strings that could be wrongly changed easily everywhere.

Test plan for issue:

Unit tests: done, passed.
Integration tests: manually tested on INT through the following steps:

Deployed this PR on INT pipeline.
Created a test cluster.
Checked that operator flags on the test cluster were set as expected, comparing them to the flags on the PR.
Also checked that operator flags matched the expected on CosmosDB.
On both cases the flags were set as expected.

✅

Is there any documentation that needs to be updated for this PR?

No documentation changes needed.

niontive · 2023-12-15T17:23:38Z

Let's revert all changes to the Python code. That code already has the licenses stuff.

test/e2e/cluster.go

test/e2e/operator.go

pkg/util/mocks/env/core.go

niontive · 2023-12-15T17:30:14Z

We can get rid of controllerEnabled consts now since we have a central place where these are defined.

pkg/operator/controllers/alertwebhook/alertwebhook_controller_test.go

pkg/operator/controllers/guardrails/guardrails_config.go

dem4gus

Thank you for this change, it was very needed.

dem4gus · 2023-12-20T19:29:01Z

pkg/operator/flags.go

+	AzureSubnetsServiceEndpointManaged = "aro.azuresubnets.serviceendpoint.managed"
+	BannerEnabled                      = "aro.banner.enabled"
+	CheckerEnabled                     = "aro.checker.enabled"
+	DnsMasqEnabled                     = "aro.dnsmasq.enabled"


nit: Dnsmasq isn't camelCase. It's technically dnsmasq but Go wants exported things to start with a capital letter.

I've changed it both for DnsmasqEnabled and RestardDnsmasqEnabled, thanks for pointing out :)

hawkowl

I'm working on making pkg/api/ independent of the rest of the codebase so that it can be imported in the Installer Wrapper, MIMO, etc without dragging in potentially all the RP's dependencies. Is there a way we can do this without pkg/api/ having a hard dependency on pkg/operator/?

old

hawkowl · 2024-01-02T02:56:07Z

/azp run ci

azure-pipelines · 2024-01-02T02:56:17Z

Azure Pipelines failed to run 1 pipeline(s).

hawkowl · 2024-01-03T00:10:39Z

/azp run ci

azure-pipelines · 2024-01-03T00:10:49Z

Azure Pipelines successfully started running 1 pipeline(s).

github-actions · 2024-01-03T15:02:52Z

Please rebase pull request.

hawkowl · 2024-01-04T02:15:15Z

/azp run ci

azure-pipelines · 2024-01-04T02:15:24Z

Azure Pipelines successfully started running 1 pipeline(s).

dofinn

lgtm nice clean up

* replacing usages of magic strings with flags from the subpackage * removing the //todo comment regarding the magic strings * replacing magic strings with operator constants * move DefaultOperatorFlags to operator package, inject when needed

azoppiserpa added 3 commits December 14, 2023 00:16

replacing usages of magic strings with flags from the subpackage

39f8f33

removing the //todo comment regarding the magic strings

28390ae

replacing magic strings with operator constants

83d2ab2

niontive reviewed Dec 15, 2023

View reviewed changes

test/e2e/cluster.go Outdated Show resolved Hide resolved

niontive reviewed Dec 15, 2023

View reviewed changes

test/e2e/cluster.go Outdated Show resolved Hide resolved

niontive reviewed Dec 15, 2023

View reviewed changes

test/e2e/operator.go Outdated Show resolved Hide resolved

niontive reviewed Dec 15, 2023

View reviewed changes

pkg/util/mocks/env/core.go Outdated Show resolved Hide resolved

azoppiserpa added 21 commits December 15, 2023 11:48

removing incorrect license from python files

84b5b18

removing incorrect license

d301f03

removing incorrect license

e2e678c

removing incorrect license

ff166fb

removing license

7d34304

removing license

c703628

removing license

ecb0cf1

removing license

b5375d8

removing license

a778eeb

removing license

b336013

removing incorrect space

27eab31

using the new flags - 1

96934da

using the new flags - 2

08d1b80

using the new flags - 3

4e7942d

removing unused consts as requested by linter

f5075eb

Removing controllerEnabled variable

5f83d92

Removing controllerEnabled

500da37

removing controllerEnabled

47dc898

removing controllerEnabled

fd6365a

replacing controllerEnabled consts with operator flags

118a011

replacing controllerEnabled consts with operator flags

b70c04e

azoppiserpa requested a review from hlipsig as a code owner December 19, 2023 19:04

azoppiserpa added 3 commits December 19, 2023 11:19

Ran make generate to fix ci-go pipeline failure

0cbb9c6

Correcting DnsMasq to Dnsmasq

d935804

Correcting DnsMasq to Dnsmasq, now for the restart flag

e7d253e

gouthamMN reviewed Dec 20, 2023

View reviewed changes

pkg/operator/controllers/alertwebhook/alertwebhook_controller_test.go Show resolved Hide resolved

pkg/operator/controllers/guardrails/guardrails_config.go Show resolved Hide resolved

dem4gus reviewed Dec 20, 2023

View reviewed changes

dem4gus previously approved these changes Dec 20, 2023

View reviewed changes

azoppiserpa dismissed dem4gus’s stale review via d935804 December 20, 2023 22:58

hawkowl previously requested changes Dec 21, 2023

View reviewed changes

hawkowl and others added 4 commits December 22, 2023 11:46

move DefaultOperatorFlags to operator package, inject when needed

cbc377b

update to inject

de1416e

fix typo

c54a120

Fixing e2e test

0602d11

github-actions bot added the needs-rebase branch needs a rebase label Jan 3, 2024

Merge remote-tracking branch 'upstream/master' into azoppiserpa/bugfix1

3432351

azoppiserpa requested review from jaitaiwan and dofinn as code owners January 4, 2024 01:32

github-actions bot removed the needs-rebase branch needs a rebase label Jan 4, 2024

dofinn approved these changes Jan 4, 2024

View reviewed changes

hawkowl merged commit 40e7987 into Azure:master Jan 4, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing magic strings on operator flags #3327

Fixing magic strings on operator flags #3327

azoppiserpa commented Dec 15, 2023 •

edited

Loading

niontive commented Dec 15, 2023

niontive commented Dec 15, 2023

dem4gus left a comment

dem4gus Dec 20, 2023

azoppiserpa Dec 20, 2023

hawkowl left a comment

hawkowl commented Jan 2, 2024

azure-pipelines bot commented Jan 2, 2024

hawkowl commented Jan 3, 2024

azure-pipelines bot commented Jan 3, 2024

github-actions bot commented Jan 3, 2024

hawkowl commented Jan 4, 2024

azure-pipelines bot commented Jan 4, 2024

dofinn left a comment

Fixing magic strings on operator flags #3327

Fixing magic strings on operator flags #3327

Conversation

azoppiserpa commented Dec 15, 2023 • edited Loading

Which issue this PR addresses:

What this PR does / why we need it:

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

niontive commented Dec 15, 2023

niontive commented Dec 15, 2023

dem4gus left a comment

Choose a reason for hiding this comment

dem4gus Dec 20, 2023

Choose a reason for hiding this comment

azoppiserpa Dec 20, 2023

Choose a reason for hiding this comment

hawkowl left a comment

Choose a reason for hiding this comment

hawkowl commented Jan 2, 2024

azure-pipelines bot commented Jan 2, 2024

hawkowl commented Jan 3, 2024

azure-pipelines bot commented Jan 3, 2024

github-actions bot commented Jan 3, 2024

hawkowl commented Jan 4, 2024

azure-pipelines bot commented Jan 4, 2024

dofinn left a comment

Choose a reason for hiding this comment

azoppiserpa commented Dec 15, 2023 •

edited

Loading