ability to download the calculation by id or value #2

odvarkadaniel · 2021-06-28T13:19:43Z

Hi,
don't know if we want to create just some kind of txt file or something. For now I just create new file with the name of calculation or it's teff and logG values.
@droslean

Signed-off-by: Daniel Odvarka odvarkaadaniel@gmail.com

cmd/ccboc/main.go

droslean · 2021-06-28T18:47:09Z

cmd/ccboc/main.go

+
+	fileName := fmt.Sprintf("%0.1f___%0.1f", teff, logG)
+
+	file, err := os.Create(fileName)


First, here we should check the error. Second, this will create the file in the current directory. We should have a default location for the downloaded results. Maybe in the same path where we create the configs for this tool?

Somehow, I am unable to create the file in another directory. Tried all sorts of functions, different permission..
os.Chdir is not doing the same thing as terminal for me. Couldnt find anything that would help me solve this.

'~/.config/ccboc' nor any other is not working. Only in $pwd. I even tried to jump between the $pwd but that didnt work either.

This should be related to your environment only. I guess permissions? You should be able to write in the ~/.config/ccboc since we already wrote the configuration files.

droslean · 2021-06-28T18:48:14Z

cmd/ccboc/main.go

+
+	_, err = file.WriteString(string(body))
+	if err != nil {
+		logrus.WithError(err).Fatal("Couldn't write the calculation into the file.")


The error message is wrong. Those data are not a calculation but bytes from a tar file.

droslean · 2021-06-28T18:48:30Z

cmd/ccboc/main.go

+	args := os.Args
+	calcID := args[len(args)-1]
+
+	fmt.Println(globalConfig.APIURL + "/calculations/results/" + calcID)


debugging leftover too?

droslean · 2021-06-28T18:50:04Z

cmd/ccboc/main.go

+		logrus.WithFields(logrus.Fields{"message": responseError.Message, "status_code": responseError.StatusCode}).Fatal("errors occurred")
+	}
+
+	file, err := os.Create(calcID)


Same here. The file is a tar.gz.

Also you are not checking the error here.

droslean · 2021-06-28T18:50:29Z

cmd/ccboc/main.go

+
+	defer file.Close()
+
+	_, err = file.WriteString(string(body))


file.Write(body)

odvarkadaniel · 2021-06-29T16:39:08Z

Hi,
added a new flag for user to enter a specific path for the download.
Also not entirely sure about the files. Its just not working on my system so I let it be this way.

droslean · 2021-07-03T17:02:04Z

cmd/ccboc/main.go

+	if path != "" {
+		defaultPath = filepath.Join(path, filepath.Base(key))
+	} else {
+		defaultPath = filepath.Join("~/.config/ccboc/", filepath.Base("text.txt"))


We should try to get the $HOME path like

user, err := user.Current() if err != nil { logrus.WithError(err).Fatal("couldn't get $HOME directory from current user") } home := user.HomeDir

Also, filepath.Base("text.txt") will always return text.txt but it is also wrong since the file will never be just a text file.

droslean · 2021-07-03T17:04:06Z

cmd/ccboc/main.go

+	}
+
+	if _, err := os.Stat(defaultPath); os.IsNotExist(err) {
+		logrus.WithError(err).Fatal("The desired file path does not exist.")


Let us have a better error message here. Also, I think it will be better to just print a warning here and then download the file to the default path.

droslean · 2021-07-03T17:04:40Z

cmd/ccboc/main.go

+	if path != "" {
+		defaultPath = filepath.Join(path, filepath.Base(calcID))
+	} else {
+		defaultPath = filepath.Join("~/.config/ccboc/", filepath.Base(calcID))


droslean · 2021-07-03T17:05:54Z

cmd/ccboc/main.go

@@ -222,6 +304,11 @@ func main() {
 				Name:  "logG",
 				Usage: "specifies the logG value when creating a calculation.",
 			},
+			&cli.StringFlag{
+				Name:    "path",


argument named path is unclear what it does. Let us have a better name, like results-download-path

droslean · 2021-07-03T17:07:58Z

cmd/ccboc/main.go

@@ -249,6 +336,20 @@ func main() {
 						Usage:   "",
 						Action:  getCalculations,
 					},
+
+					{


lets just have one command that belongs in the calculation command. I would expect for the user to just type ccboc get calculation results {id} or ccboc --logG=4.0 --teff=1000 get calculation results

@gszasz We could use your insight as a user in this one...

odvarkadaniel · 2021-07-03T19:06:39Z

@droslean Not sure about the implementation of the cli commands. Couldnt find some kind of example of this kind of thing.
Also, is the "filename" header OK? I cant even recieve for example Content-Type just to check.

droslean · 2021-07-03T21:03:18Z

@odvarkadaniel

Not sure about the implementation of the cli commands. Couldnt find some kind of example of this kind of thing.

You can check the library in godoc.

Also, is the "filename" header OK?

Let's have some tests that can check that.

odvarkadaniel · 2021-07-04T10:20:15Z

@droslean Its just not letting me get to the headers locally. Dont know what I can do about it.

error="Get "https://apiserver-vega.router.default.svc.cluster.local/calculations/results?teff=9000.0&logg=4.0\": x509: certificate signed by unknown authority"

Hopefully, this isnt some stupid way to get to the headers:)
Could we schedule a meeting about all this + dont understand some of this process - the files we are creating etc.

Also, is the "filename" header OK?

Let's have some tests that can check that.

droslean · 2021-07-04T15:27:33Z

You still trying to reach the API server in the cluster. Why not just testing it locally?

odvarkadaniel · 2021-07-04T15:47:24Z

You still trying to reach the API server in the cluster. Why not just testing it locally?

How do I reach it locally? I have no clue what you would mean by that..

droslean · 2021-07-06T11:26:34Z

How do I reach it locally? I have no clue what you would mean by that..

Run the apiserver locally. It has the dry-run, for the get/create/delete calculations but you don't even care about that. You can just start it by pointing to your results folder, add the fort.7 and fort.8 files in there, and expect to get them.

Example results folder structure:
results/9000.0___4.00/fort.7
results/9000.0___4.00/fort.8

odvarkadaniel · 2021-07-23T16:17:39Z

Hi,
if user wants to download the result by teff and logG, or just by calcID, the tar.gz file get created as the name of the header (10071.0_4.00-results.tar.gz). There are the two fort files - fort.7 and fort.8
@droslean

droslean · 2021-07-23T18:10:32Z

cmd/ccboc/main.go

+	if path != "" {
+		defaultPath = filepath.Join(path, filepath.Base(newKey[1]))
+	} else {
+		logrus.Info("Desired path to download the calculations was not included. The calculations are set to be downloaded into: $HOME/.config/ccboc/{id}")


no need for this log. Instead, just log where we downloaded the results.

i think the user should know, that the path was not added properly and was not found.

droslean · 2021-07-23T18:12:06Z

cmd/ccboc/main.go

+	}
+
+	key := resp.Header.Get("Content-Disposition")
+	newKey := regexp.MustCompile("=").Split(key, 2)


Please use strings.Split() instead.

droslean · 2021-07-23T18:14:36Z

cmd/ccboc/main.go

+
+	var defaultPath string
+
+	user, err := user.Current()


just use conig.GetDefaultConfigPath()

We should be able to just have

defaultPath := path if defaultPath == "" { defaultPath, err = config.GetDefaultConfigPath() if err != nil{...} }

This rewrites the config file, which causes errors.
DefalultConfigName gets thrown in and generates at defaultPath.

Just created a new function really which is pretty much the same thing as this one, but with different fileName.

droslean · 2021-07-23T18:24:29Z

cmd/ccboc/main.go

+		defaultPath = filepath.Join(home, "/.config/ccboc/", filepath.Base(newKey[1]))
+	}
+
+	if _, err := os.Stat(defaultPath); !errors.Is(err, os.ErrNotExist) {


lets have a helper func that creates the results folder in the config package

droslean · 2021-07-26T10:08:46Z

pkg/config/config.go

@@ -49,3 +50,28 @@ func GetDefaultConfigPath() (string, error) {

 	return filepath.Join(home, DefaultConfigPath, DefaultConfigFileName), nil
 }
+
+func GetDefaultPath(fileName string) (string, error) {


This new function is a duplicate with GetDefaultConfigPath. Let's refactor it to get the desired location instead.

Just had to change the name of the file, because 'DefaultConfigName' is set to 'config' and it brakes the config file.

Yea, but we still have duplicated code in this function. Instead refactor GetDefaultConfigPath.

droslean · 2021-07-26T10:12:31Z

cmd/ccboc/main.go

+		logrus.WithError(err).Fatal("Couldn't retrieve the data from headers.")
+	}
+
+	key := resp.Header.Get("Content-Disposition")


lets use better names rather than key and newKey

odvarkadaniel · 2021-07-27T08:12:51Z

@droslean Function is refactored, variables renamed. Everything seems to be working. Exception errors handled.

droslean · 2021-07-27T11:20:00Z

cmd/ccboc/main.go

+
+	fileNameHeader := resp.Header.Get("Content-Disposition")
+	splitFileNameHeader := strings.Split(fileNameHeader, "=")
+	if len(splitFileNameHeader) == 1 {


Header.Get() will return "" if there is no header. You can make the check earlier.

See https://pkg.go.dev/net/http#Header.Get

droslean · 2021-07-27T11:22:04Z

pkg/config/config.go

+
+	if defaultPath == "" {
+		return filepath.Join(home, DefaultConfigPath, fileName), nil
+	} else {


There is no need for the else here.

droslean · 2021-07-27T11:23:17Z

pkg/config/config.go

+			logrus.Info("Desired path to download the calculations does not exist. The calculations are set to be downloaded into: $HOME/.config/ccboc/{id}")
+			return filepath.Join(home, "/.config/ccboc/", filepath.Base(fileName)), nil
+		}
+		defaultPath = filepath.Join(defaultPath, filepath.Base(fileName))


There is no need to manipulate this variable that was passed in the function already.

droslean · 2021-07-27T11:28:26Z

pkg/config/config.go

+		return filepath.Join(home, DefaultConfigPath, fileName), nil
+	} else {
+		if _, err := os.Stat(defaultPath); os.IsNotExist(err) {
+			logrus.Info("Desired path to download the calculations does not exist. The calculations are set to be downloaded into: $HOME/.config/ccboc/{id}")


When logging, the user should know all the information about the problem. The desired path is unclear here. We need to give him the exact path. Also, by handling only the not exists error, we eat all the other potential errors. Instead lets check for all errors and output the error in the screen. There is no need to fallback to the default location, since the user didn't ask for that.

Changed it to errors.Is(err, os.ErrNotExist). Hopefully that's enough.

No I mean we should check all the errors, and output the error to the user. Don't fall back to the default location.

Well, thats what I did in the latest commit. When there is an error, then we fail. I dont fall to the def. location anymore.

Yes, but you are checking only one error case. We need to fail in any error.

droslean · 2021-07-27T13:18:46Z

pkg/config/config.go

+
+	_, err = os.Stat(defaultPath)
+	if err != nil {
+		return "", fmt.Errorf("something is wrong with the download path of the calculation %v", defaultPath)


There is no reason to eat the actual error message with a something is wrong message. We need the user to know exactly why his actions failed. We need something like:

Suggested change

return "", fmt.Errorf("something is wrong with the download path of the calculation %v", defaultPath)

return "", fmt.Errorf("couldn't stat path %s: %v", defaultPath, err)

droslean · 2021-07-27T13:26:59Z

cmd/ccboc/main.go

+
+	fileNameHeader := resp.Header.Get("Content-Disposition")
+	if fileNameHeader == "" {
+		logrus.WithError(err).Fatal("Unable to get the headers")


The log here is wrong. The err will be nil since there is no error. Also please be more detailed about which header we failed to get.

droslean · 2021-07-27T13:27:25Z

cmd/ccboc/main.go

+
+	fileNameHeader := resp.Header.Get("Content-Disposition")
+	if fileNameHeader == "" {
+		logrus.WithError(err).Fatal("Unable to get the headers")


Signed-off-by: Daniel Odvarka <odvarkaadaniel@gmail.com>

droslean requested changes Jun 28, 2021

View reviewed changes

odvarkadaniel force-pushed the download-calc branch from b6ff85a to bd1cd78 Compare June 29, 2021 16:37

droslean reviewed Jul 3, 2021

View reviewed changes

odvarkadaniel force-pushed the download-calc branch 2 times, most recently from c5bac40 to 40cb614 Compare July 3, 2021 19:05

odvarkadaniel force-pushed the download-calc branch 2 times, most recently from 03dc837 to b3d39ea Compare July 23, 2021 16:17

droslean reviewed Jul 23, 2021

View reviewed changes

odvarkadaniel force-pushed the download-calc branch from b3d39ea to 43c64d3 Compare July 23, 2021 19:20

droslean reviewed Jul 26, 2021

View reviewed changes

odvarkadaniel force-pushed the download-calc branch 2 times, most recently from 02b5f14 to 1c2a615 Compare July 26, 2021 11:12

droslean reviewed Jul 27, 2021

View reviewed changes

odvarkadaniel force-pushed the download-calc branch 2 times, most recently from 2f0021c to e8bc6cf Compare July 27, 2021 13:13

droslean reviewed Jul 27, 2021

View reviewed changes

odvarkadaniel force-pushed the download-calc branch from e8bc6cf to ec3289e Compare July 27, 2021 13:21

droslean reviewed Jul 27, 2021

View reviewed changes

odvarkadaniel force-pushed the download-calc branch from ec3289e to fb3fe38 Compare July 27, 2021 13:31

ability to download the calculation by id or value

26876b2

Signed-off-by: Daniel Odvarka <odvarkaadaniel@gmail.com>

odvarkadaniel force-pushed the download-calc branch from fb3fe38 to 26876b2 Compare July 28, 2021 10:49

droslean merged commit b2e9d58 into vega-project:main Jul 28, 2021

odvarkadaniel deleted the download-calc branch July 28, 2021 11:09


		fileName := fmt.Sprintf("%0.1f___%0.1f", teff, logG)

		file, err := os.Create(fileName)

	return "", fmt.Errorf("something is wrong with the download path of the calculation %v", defaultPath)
	return "", fmt.Errorf("couldn't stat path %s: %v", defaultPath, err)

ability to download the calculation by id or value #2

ability to download the calculation by id or value #2

Conversation

odvarkadaniel commented Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

odvarkadaniel Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odvarkadaniel commented Jun 29, 2021

droslean Jul 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odvarkadaniel commented Jul 3, 2021

droslean commented Jul 3, 2021

odvarkadaniel commented Jul 4, 2021 • edited Loading

droslean commented Jul 4, 2021

odvarkadaniel commented Jul 4, 2021

droslean commented Jul 6, 2021 • edited Loading

odvarkadaniel commented Jul 23, 2021

droslean Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odvarkadaniel Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droslean Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odvarkadaniel commented Jul 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odvarkadaniel Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odvarkadaniel commented Jun 28, 2021 •

edited

Loading

odvarkadaniel Jun 28, 2021 •

edited

Loading

droslean Jul 3, 2021 •

edited

Loading

odvarkadaniel commented Jul 4, 2021 •

edited

Loading

droslean commented Jul 6, 2021 •

edited

Loading

droslean Jul 23, 2021 •

edited

Loading

odvarkadaniel Jul 23, 2021 •

edited

Loading

droslean Jul 26, 2021 •

edited

Loading

odvarkadaniel Jul 27, 2021 •

edited

Loading