Skip to content
This repository has been archived by the owner on May 7, 2023. It is now read-only.

git repository #36

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

git repository #36

wants to merge 4 commits into from

Conversation

Neyrii
Copy link
Contributor

@Neyrii Neyrii commented Oct 20, 2022

First version

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

url = f'{remote_host}/api/v2/settings/general'
host_name = urlparse(remote_host).hostname
res = requests.get(url, headers={"Authorization": f"Bearer {token}"})
if res.status_code == 401:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this check we have to verify that the status code is actually 200 and raise an exception if it's not like "Failed to connect to Giskard instance"

host_name = urlparse(remote_host).hostname
res = requests.get(url, headers={"Authorization": f"Bearer {token}"})
if res.status_code == 401:
raise Exception("Wrong Token") # Not shure of what exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Invalid API Token" seems like a good option



def clone_git_testing_repository(instance_id: int, is_silent: bool):
instance_path = os.path.expanduser(f'{settings.home}/{str(instance_id)}')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead do:

Path(expand_env_var(settings.home)) / str(instance_id)

in this case it it won't rely on unix forward slash

import os
import logging
import click
from git import Repo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't forget to add git to pyproject.toml

giskard/cli.py Outdated


def _start_command(is_server, host, port, is_daemon):
def _start_command(is_server, is_silent, host, port, is_daemon):
token = click.prompt("Please enter an API Access Token")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of prompting this value directly it's better to declare it as another cli option with a prompt property
like so
https://click.palletsprojects.com/en/8.1.x/options/#prompting

@andreybavt
Copy link
Contributor

There's one unhandled case - the worker started in a "server" mode with -s argument

In this case it's not worker that connects to Giskard server, but vice versa. So we can't sent an initialization HTTP request and get an instance id back. I suggest that we switch the -s argument from a flag to a regular argument that accepts a value of an instance id.
For example when you run

giskard worker start -s o19fnwjikjioyr0j it'll start in server mode and will use giskard-home/o19fnwjikjioyr0j as a base directory

Also in case a worker is started in server mode there's no need to setup the git repo since at start time giskard host is not known

@andreybavt andreybavt modified the milestones: 1.4.0, 1.8.0 Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants