-
-
Notifications
You must be signed in to change notification settings - Fork 46
codify custom search attributes for TemporalNamespace #776
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
base: main
Are you sure you want to change the base?
codify custom search attributes for TemporalNamespace #776
Conversation
TODO(@TheHiddenLayer) add tests? |
Hi @alexandrevilain,
P.S. regarding:
the error is different each time. Here's the error I happened to see this time:
|
This is running on an M3 apple silicon with fails again, this time with a different error.
|
even while running the
@alexandrevilain are you able to run the whole e3e test suite without any errors or timeouts? |
} | ||
} | ||
|
||
err = r.reconcileCustomSearchAttributes(ctx, &logger, namespace, cluster) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexandrevilain is this the right place to start reconciling custom search attribtues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I should wait for the namespace to be marked ready, before reconciling custom search attributes, right? But where should I put that waiting logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my current implementation is causing the TemporalNamespace_ready
e2e test to fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is the good place for this :)
Sorry @TheHiddenLayer I'm currently having issues with internet connectivity while I'm in holidays. I can't even pull the cert-manager image ... |
Hi @TheHiddenLayer following PR succesfully ended e2e without any retry: #777 |
Nice, I'm glad that it worked. It could be something with my logic then? Pls LMK what you think about this. |
logger := log.FromContext(ctx) | ||
|
||
logger.Info("Starting reconciliation") | ||
logger.Info("=== (NS) STARTING RECONCILIATION") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're ok on the fast that those kind of logging will be removed once the code is working, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly. These logging statements are for my debugging. I pushed them up to keep my branch up to date. Unfortunately, they show up in the PR.
} | ||
} | ||
|
||
err = r.reconcileCustomSearchAttributes(ctx, &logger, namespace, cluster) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is the good place for this :)
@alexandrevilain I haven't forgotten about this PR. I'm still planning to finish it off. I’ve been tied up lately with work. Thanks for your patience! |
|
Hi @TheHiddenLayer ! |
hey @alexandrevilain! the feature itself works as expected. I've done manual tests confirming:
BUT I didn't deeply investigate why this doesn't pass some E2E tests, so any assistance on that would be great. I wanted to filter out and run a few tests that were failing to isolate my debugging, but the option flags I passed into the test command didn't do anything. So, the fact that I had to run the entire E2E pipeline (30-50min on my computer) has made this arduous to debug. |
|
I'll clean up the comments/dead code/debugging log statements |
Hey guys I realise last update on this was quite a while ago but I am rather keen to this merged. Is there something I can do to help wrap this up? |
@AdamSendible if you can get the e2e tests to work it should be merge-able :) I wasn't able to get it to work |
Closes #567
This PR codifies custom search attributes, so one can specify: