improvement(SCTParameterWizard): add validation for Scylla BYO install#698
improvement(SCTParameterWizard): add validation for Scylla BYO install#698jsmolar wants to merge 1 commit into
Conversation
| } | ||
|
|
||
| def validate_repo(self, repo: str, branch: str): | ||
| try: |
There was a problem hiding this comment.
This is ugly. I was considering using code from jenkins_service.py instead. Don't know what is better:
response = requests.get(
url=f"https://api.github.com/repos/{user}/{repo}/contents/{new_settings['pipelineFile']}?ref={new_settings['gitBranch']}",
headers={
"Accept": "application/vnd.github+json",
"Authorization": f"Bearer {token}",
}
)
if response.status_code == 404:
return (False, f"Pipeline file not found in the <a href=\"https://github.com/{user}/{repo}/tree/{new_settings['gitBranch']}\"> target repository</a>, please check the repository before continuing")
if response.status_code == 403:
return (True, "No access to this repository using your token. The pipeline file cannot be verified.")
if response.status_code == 200:
return (True, "")
There was a problem hiding this comment.
I think GithubException in this context is not very interesting to handle as it's just slightly better formatted message, this branch can be removed.
| internalName: "byo_scylla_repo", | ||
| requiresValidation: true, | ||
| validated: true, | ||
| validated: false, |
There was a problem hiding this comment.
Without updating validate function of this object, the validation result from validate_BYO_repo_params will be ignored when user presses Build button, it would be better to use this object state instead of a separate repoValidation object to do validation calls. (validationHelpText can be dynamically updated).
validate function will be required to switch to async however, as well as handleParameterSubmit in ParameterEditor.svelte
There was a problem hiding this comment.
validated parameter is used in this PR just for disabled={repoValidation.inProgress || !paramDefinitions.scyllaVersion.params.scyllaBranchBYO.validated || !paramDefinitions.scyllaVersion.params.scyllaRepoBYO.validated} where I deactivate the Validate button when the wizard is first shown with empty BYO fields.
My idea here was that scyllaRepoBYO and scyllaBranchBYO need to by validated first by validate function. After that they can manually click validate button to verify if repo exists.
There was a problem hiding this comment.
I changed it slightly (see the condition for validation button disable) so I keep default validated: true as the rest of the params.
pehala
left a comment
pehala
left a comment
There was a problem hiding this comment.
How can the image from cover letter be valid? does git@github.com:personal-username/scylla.git exist?
|
@jsmolar please fix the conflicts |
|
@k0machi please re-review |
I'll pick this up later and fix, this PR is still trying to go around the normal validation routine instead of registering validation function for those new fields and I believe will still not fail form submit if validation fails (or at least partially reimplements validation mechanism) |
|
@k0machi remember about this one |
4 participants
Similar to Clone run/job, this PR adds validation for the GitHub repository and branch.