-
Notifications
You must be signed in to change notification settings - Fork 353
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
Add redpanda connect secret manager to console #1527
Conversation
359c7df
to
1e0317a
Compare
const getDefaultView = (defaultView: string): { initialTab: ConnectView, redpandaConnectTab: ConnectView } => { | ||
|
||
const showPipelines = Features.pipelinesApi | ||
const showKafkaTab = {initialTab: ConnectView.kafkaConnect, redpandaConnectTab: ConnectView.RedpandaConnect} | ||
const showRedpandaConnectTab = {initialTab: ConnectView.RedpandaConnect, redpandaConnectTab: ConnectView.RedpandaConnect} |
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.
Nothing to say about this change but overall we desperately need a linter 😓 I will try to put something together when I have spare 5 mins
case 'redpanda-connect': | ||
return showRedpandaConnectTab; | ||
case 'redpanda-connect-secret': | ||
return {initialTab: ConnectView.RedpandaConnect, redpandaConnectTab: ConnectView.RedpandaConncetSecret}; |
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.
RedpandaConncetSecret
-> RedpandaConnectSecret
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.
fix
import { useLocation } from 'react-router-dom'; | ||
|
||
enum ConnectView { | ||
kafkaConnect = 'kafka-connect', |
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.
Let's make it uppercase to match the other values
kafkaConnect
-> KafkaConnect
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.
fix
const {search}= useLocation(); | ||
const searchParams = new URLSearchParams(search); | ||
const defaultTab = searchParams.get('defaultTab') || ''; | ||
return <KafkaConnectOverview defaultView={defaultTab} {...props}/> |
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.
What happens if defaultTab
is ''
? I think we need to return to tabConnectView.RedpandaConnect
by default?
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.
it should default from getDefaultView()
startLineNumber: position.lineNumber, | ||
endLineNumber: position.lineNumber, |
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.
It's always going to start and end on the same line, is that intentional?
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.
yes, it is intencional, but I disabled autocomplete, because it needs more testing and there isn't time now
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.
remove this function for now, I need to improve the autocomplete
const last_chars = model.getValueInRange({startLineNumber: position.lineNumber, startColumn: 0, endLineNumber: position.lineNumber, endColumn: position.column}); | ||
const words = last_chars.replace('\t', '').replace('\{', '').split(' '); | ||
const active_typing = words[words.length - 1]; | ||
const empty = {suggestions: []} |
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.
Maybe we can continue using camelCase
<PageContent> | ||
<ToastContainer/> | ||
<Flex flexDirection="column" gap={5}> | ||
<FormField label="Secret name" isInvalid={alreadyExists} errorText="Secret name is already in use"> |
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.
Is that always going to be the reason for the error? What if some invalid character is used? Wouldn't it show wrong error text?
if (String(err).includes('404')) { | ||
// Hacky special handling for OSS version, it is possible for the /endpoints request to not complete in time for this to render | ||
// so in this case there would be an error shown because we were too fast (with rendering, or the req was too slow) | ||
// We don't want to show an error in that case | ||
return; | ||
} |
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 maybe it sounds worse than it is, but thanks for explaining 😬
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 copy and past this handle.
return ( | ||
<PageContent> | ||
<Section> | ||
<ToastContainer/> |
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.
Do we need ToastContainer
copied everywhere? There's no global provider?
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.
Again, I use pipeline as model, we can improve it later, when we have all console context
|
||
initPage(p: PageInitHelper) { | ||
p.title = 'Update Secret'; | ||
p.addBreadcrumb('Redpanda Connect Secret Manager', '/rp-connect/secret/update'); |
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.
Note: I think in the future we should have a global place with all the routes to ensure we can easily change name/forbid accessing it across the app if we needed to. Just in the router file there should be a place to get all the route paths.
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 don't know what was the reason for this change, we can discuss it later
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.
Should it be secret or secrets?
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.
it should be secrets, I fixing it
<Input | ||
placeholder="Enter a secret name..." | ||
data-testid="secretId" | ||
pattern="[a-zA-Z0-9_\-]+" |
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.
Maybe we can reuse this regex for secrets, actually I see this pattern is different
^[A-Z][A-Z0-9_]*$
vs [a-zA-Z0-9_\-]+
, which one should we use?
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 updated this based on proto
frontend/src/state/ui.ts
Outdated
@@ -234,6 +234,10 @@ const defaultUiSettings = { | |||
quickSearch: '' | |||
}, | |||
|
|||
rpncSecretList: { |
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 you meant
rpcnSecretList
-> rpncSecretList
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.
right
1e0317a
to
b329bba
Compare
|
||
|
||
@observer | ||
class RpConnectSecretCreate extends PageComponent { |
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.
When we try to load these new pages and RP-connect is not enabled, we get infinite loading, this applies for both new URLs that have been added to the router. Can we please show something that prompts the user to configure it?
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.
when it happens, rpcn-secret continue working:
frontend/src/components/pages/rp-connect/secrets/Secrets.Create.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/pages/rp-connect/secrets/Secrets.Create.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/pages/rp-connect/secrets/Secrets.Create.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/pages/rp-connect/secrets/Secrets.Create.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/pages/rp-connect/secrets/Secrets.List.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/pages/rp-connect/secrets/Secrets.List.tsx
Outdated
Show resolved
Hide resolved
Maybe it doesn't have to be part of this PR, but it's related, is there any way we can add the well known predefined contextual env vars to auto completion. The env vars added in https://github.com/redpanda-data/cloudv2/pull/18566. |
Good idea, we need to check how we can add it on autocomplete, I disabled autocomplete because I want to spend more time on it |
This PR introduces a new tab for Redpanda Connect
Features:
defaultTab
Pending:
Dedicated integration cluster
Screen.Recording.2024-11-21.at.8.43.30.PM.mov
Serverless integration cluster
Screen.Recording.2024-11-26.at.8.41.10.AM.mov
Before:
Enterprice self-hosted
Screen.Recording.2024-11-25.at.7.42.10.PM.mov