-
Notifications
You must be signed in to change notification settings - Fork 5
fix: Exec and Jenkins Job Agent #408
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
Changes from all commits
199306f
9a43cfd
471be86
beba1fd
c347ad3
671eb6d
08d33ac
6021d32
4f62515
d402802
9f65b18
7896205
88da119
7927eb2
0fba5e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
"use client"; | ||
|
||
import { Button } from "@ctrlplane/ui/button"; | ||
import { FormDescription, FormItem, FormLabel } from "@ctrlplane/ui/form"; | ||
import { Input } from "@ctrlplane/ui/input"; | ||
|
||
interface JobAgentJenkinsPipelineConfigProps { | ||
value: Record<string, any>; | ||
onChange: (value: Record<string, any>) => void; | ||
disabled?: boolean; | ||
isPending?: boolean; | ||
} | ||
|
||
export const JobAgentJenkinsPipelineConfig: React.FC< | ||
JobAgentJenkinsPipelineConfigProps | ||
> = ({ value, onChange, disabled, isPending }) => { | ||
const handleJobUrlChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
onChange({ | ||
...value, | ||
jobUrl: e.target.value, | ||
}); | ||
}; | ||
zacharyblasczyk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return ( | ||
<div className="space-y-4"> | ||
<FormItem> | ||
<FormLabel>Jenkins Job URL</FormLabel> | ||
<Input | ||
placeholder="e.g. http://jenkins/job/org/job/repo/job/branch" | ||
value={value.jobUrl ?? ""} | ||
onChange={handleJobUrlChange} | ||
disabled={disabled} | ||
/> | ||
Comment on lines
+28
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add URL validation for the Jenkins job URL. The component provides a format description but lacks validation to ensure users enter a properly formatted Jenkins URL. <Input
placeholder="e.g. http://jenkins/job/org/job/repo/job/branch"
value={value.jobUrl ?? ""}
onChange={handleJobUrlChange}
disabled={disabled}
+ pattern="https?:\/\/.*\/job\/.*\/job\/.*\/job\/.*"
+ title="Please enter a valid Jenkins URL format: http://jenkins/job/org/job/repo/job/branch"
/> Alternatively, you could implement a custom validation function and display error messages when the URL doesn't match the expected format: const isValidJenkinsUrl = (url: string) => {
return /https?:\/\/.*\/job\/.*\/job\/.*\/job\/.*/.test(url);
};
// Add this to your component
const [isValid, setIsValid] = useState(true);
const handleJobUrlChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const newValue = e.target.value;
setIsValid(newValue === '' || isValidJenkinsUrl(newValue));
onChange({
...value,
jobUrl: newValue,
});
};
// Then add error indication to your Input or below it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't the only pattern that jobs can come it. So I am not worried about validating input now. If the jenkins server is available, we might have a different form altogether where we just let them pick from a dropdown, but since we are focused on airgapped instances, I am just keeping it simple for now. I will probably want something more like: DeploymentJobAgentGithubConfig.tsx @coderabbitai, please create an issue as a feature request describing what I have laid out here and a proposal of how to handle the two cases airgapped and not airgapped. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@zacharyblasczyk I've created an issue with a feature request for enhancing the Jenkins job configuration to handle both airgapped and non-airgapped environments. The issue includes:
I've assigned the issue to you as requested. Let me know if you'd like any changes to the description or scope of the feature request. 🏁 Script executed:
Length of output: 1078 🏁 Script executed:
Length of output: 1437 🏁 Script executed:
Length of output: 9461 🏁 Script executed:
Length of output: 1832 @zacharyblasczyk I've created a feature request issue (#478) describing the two approaches for Jenkins job configuration:
The issue includes a proposal for creating a component similar to Here's the link to the issue: #478✏️ Learnings added
|
||
<FormDescription> | ||
The URL path to the Jenkins job (format: {"{JENKINS_URL}"} | ||
/job/org/job/repo/job/branch) | ||
</FormDescription> | ||
</FormItem> | ||
|
||
<div className="flex"> | ||
<Button type="submit" disabled={disabled ?? isPending}> | ||
{isPending ? "Saving..." : "Save"} | ||
</Button> | ||
</div> | ||
</div> | ||
zacharyblasczyk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
}; |
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 this related to the jenkins stuff? we should probably keep the scope of this PR to jsut that and do this as follow up, umelss do we need this for the jenkins to work?
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.
This is needed to enable jenkins and exec agents to work properly without breaking the gh job agent.