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

Update ConfigEntries to be Partition aware #724

Merged
merged 2 commits into from
Sep 15, 2021

Conversation

thisisnotashwin
Copy link
Contributor

@thisisnotashwin thisisnotashwin commented Sep 14, 2021

Changes proposed in this PR:

  • Update ServiceDefaults and ServiceIntentions to be partition aware.

How I've tested this PR:

  • Unit tests
  • Deploying to cluster and they get reconciled successfully.

How I expect reviewers to test this PR:

  • Code review

Notes: There is a validation on Service Intentions that fails if namespaces are not enabled but the namespace field has been populated. A similar check has not been made currently as this would require changing the method signature of the ConfigEntry interface which would leads to almost every file in the /api/v1alpha1 directory getting impacted. Post the alpha release, I will do a refactor which is extract a bunch of fields required for validation and put them in a struct. This should ensure we don't have to change method signatures every time a new concept is introduced. We can update the struct and the config-entries that are impacted.

Checklist:

  • Tests added

@thisisnotashwin thisisnotashwin force-pushed the config-entries-partitions branch from 5b43e76 to 9d51a52 Compare September 14, 2021 18:26
@thisisnotashwin thisisnotashwin changed the base branch from main to partitions September 14, 2021 18:31
@thisisnotashwin thisisnotashwin force-pushed the config-entries-partitions branch 2 times, most recently from 96f3ba2 to 5d76877 Compare September 14, 2021 20:20
@thisisnotashwin thisisnotashwin force-pushed the config-entries-partitions branch 2 times, most recently from 61fd143 to 410c7ba Compare September 14, 2021 23:10
@thisisnotashwin thisisnotashwin marked this pull request as ready for review September 14, 2021 23:16
@thisisnotashwin thisisnotashwin requested review from ishustava, a team and ndhanushkodi and removed request for a team September 14, 2021 23:16
@ndhanushkodi
Copy link
Contributor

There is a validation on Service Intentions that fails if namespaces are not enabled but the namespace field has been populated.

I think this could be fine to keep around until alpha as you mentioned but I just wanted to clarify the following in case I'm misunderstanding:

Is this the failing ent unit test in CI? Would we keep the failing test until post-alpha? Would you be able to run acceptance tests on this PR by skipping the ent unit tests if those will be failing for alpha? Do we need to modify CI to skip the that unit test until post alpha and make a reminder to re-enable it?

@thisisnotashwin thisisnotashwin force-pushed the config-entries-partitions branch from 410c7ba to a1c0bc4 Compare September 15, 2021 16:59
@thisisnotashwin
Copy link
Contributor Author

There is a validation on Service Intentions that fails if namespaces are not enabled but the namespace field has been populated.

I think this could be fine to keep around until alpha as you mentioned but I just wanted to clarify the following in case I'm misunderstanding:

Is this the failing ent unit test in CI? Would we keep the failing test until post-alpha? Would you be able to run acceptance tests on this PR by skipping the ent unit tests if those will be failing for alpha? Do we need to modify CI to skip the that unit test until post alpha and make a reminder to re-enable it?

The failing enterprise test should stop failing with the latest Consul enterprise alpha image. The test that is failing is the partition-init job that currently depends on the existence of Admin Partitions. I could create a PR that skips enterprise unit tests and runs the acceptance tests to ensure that the acceptance tests are 🟢

Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Looks great Ashwin!!

The failing enterprise test should stop failing with the latest Consul enterprise alpha image.

ahh got it

I could create a PR that skips enterprise unit tests and runs the acceptance tests to ensure that the acceptance tests are green

might be a good idea, at least so we see controller acc tests go green? Maybe it doesn't have to be a blocker though

@thisisnotashwin thisisnotashwin force-pushed the config-entries-partitions branch from 9c7d273 to cae6589 Compare September 15, 2021 19:07
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looks great! I left a couple of non-blocking, but otherwise looks good. I haven't tested it though. Should we add an acceptance test to test out partitions similar to how we test namespaces for config entries?

Comment on lines +210 to +211
description: Partition is only accepted within a service-defaults
config entry.
Copy link
Contributor

Choose a reason for hiding this comment

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

This description is a bit confusing. It seems like we only say which config entries allow it. Maybe this could be a second sentence after saying what the partition property is? This is non-blocking.

consulClient, err := c.httpFlags.APIClient()
cfg := api.DefaultConfig()
c.httpFlags.MergeOntoConfig(cfg)
cfg.Partition = c.flagPartition
Copy link
Contributor

Choose a reason for hiding this comment

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

Non blocking: Could we make it so that partition is auto-parsed from flags when we call MergeOntoConfig?

@thisisnotashwin
Copy link
Contributor Author

Looks great! I left a couple of non-blocking, but otherwise looks good. I haven't tested it though. Should we add an acceptance test to test out partitions similar to how we test namespaces for config entries?

We definitely should update our acceptance tests. I was contemplating doing that as a part of the larger story of creating acceptance tests for Admin Partitions.

@thisisnotashwin thisisnotashwin force-pushed the config-entries-partitions branch from cae6589 to e71dbdf Compare September 15, 2021 21:46
@thisisnotashwin thisisnotashwin force-pushed the config-entries-partitions branch from e71dbdf to 61e4cd9 Compare September 15, 2021 22:01
@thisisnotashwin thisisnotashwin merged commit 7a1b80b into partitions Sep 15, 2021
@thisisnotashwin thisisnotashwin deleted the config-entries-partitions branch September 15, 2021 22:02
thisisnotashwin pushed a commit that referenced this pull request Sep 16, 2021
* Add partition support to config entries

* Fail if namespaces are not enabled and admin partitions are enabled
thisisnotashwin pushed a commit that referenced this pull request Sep 17, 2021
* Add partition support to config entries

* Fail if namespaces are not enabled and admin partitions are enabled
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.

3 participants