CLOUDP-221979: Atlas search index #1430
Conversation
|
I think, once the typing is completed, we could merge this as the first part of the change. Next we could review how we handle the CRDs without distractions on the type definitions. IMHO we could add PRs incrementally even for unfinished work, so far it does not break the build and incomplete code does not affect the end users, so it is invisible to them. For us the benefit is smaller and more focused PRs. |
igor-karpukhin
force-pushed
the
CLOUDP-221979/atlas-search-index
branch
from
b0ad7ad to
f1b4076
Compare
igor-karpukhin
force-pushed
the
CLOUDP-221979/atlas-search-index
branch
from
71ab979 to
114de06
Compare
igor-karpukhin
force-pushed
the
CLOUDP-221979/atlas-search-index
branch
3 times, most recently
from
615bdb1 to
f787086
Compare
|
side note @igor-karpukhin: I am generally fine with addressing the missing unit tests next week, iff e2e yields green here so we can have at least one monday-midnight e2e and matrix test signal next week. |
| if !verifyAllIndexesNamesAreUnique(sr.deployment.Spec.DeploymentSpec.SearchIndexes) { | ||
| return sr.terminate(status.SearchIndexesNamesAreNotUnique, fmt.Errorf("every index 'Name' must be unique")) | ||
| } | ||
| sr.ctx.Log.Debug("all indexes names are unique") |
There was a problem hiding this comment.
Is this intended for the final version or just a trace?
| } | ||
|
|
||
| func (r *AtlasSearchIndexConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | ||
| log := r.Log.With("atlassearchindexconfig", req.NamespacedName) |
There was a problem hiding this comment.
why not camel cased?
| log := r.Log.With("atlassearchindexconfig", req.NamespacedName) | |
| log := r.Log.With("AtlasSearchIndexConfig", req.NamespacedName) |
The logger was created with that:
Log: logger.Named("controllers").Named("AtlasSearchIndexConfig").Sugar(),
In fact, I am confused why we need both definitions. Seems that log libe above might be repetitive or stuttering, with or without camel case.
|
|
||
| // NewSearchIndexFromAtlas returns internal representation of the SearchIndex converted from Atlas | ||
| // internals. It can return an error in case some fields are not valid JSON. | ||
| func NewSearchIndexFromAtlas(index admin.ClusterSearchIndex) (*SearchIndex, error) { |
There was a problem hiding this comment.
This method has little unit test coverage, please increase as it is central. We just found a bug in here as well.
|
TLDR from the discussion with @josvazg and @helderjs: @igor-karpukhin please address comments in a follow-up PR. We will merge this one as soon as it goes green on e2e. It already passes green locally on my machine. |
All Submissions:
This PR adds
AtlasSearchfeature to the operatorcloses #XXXXin your comment to auto-close the issue that your PR fixes (if there is one).