gateway/runservice: add api to delete step logs

[camandel]

A try to fix issue #41 for logs deletion.

[sgotti]

@camandel Thanks for the PR! before reviewing, can you add some tests (as integration tests and runservice tests)?

Related agola-web issues: agola-io/agola-web#5 agola-io/agola-web#24

[camandel]

@sgotti I've add two integration tests in setup_test.go but I did not find a way to add runservice tests.
Could you indicate where to add these tests and an existing file to look at as example?

[sgotti]

@camandel The integration tests looks good. For the runservice test there isn't an existing one right now. The idea is to create a test self contained in the runservice that will create a run and then delete the logs. But this could be done in another PR.

Comments inline. Tests are failing due to golangci-lint errors.

[camandel]

@sgotti Modified to use httpError function and new errors types. Added new tests to check non existing logs.

[sgotti]

@camandel Thanks. Looks like TestDirectRunLogs is failing.

[camandel]

I tried it in my env (all-in-one server with docker executor) but the test TestDirectRunLogs/test_delete_log_step_1passed all the times.
The error log delete error: Log for task XXXX in run YYYY is not yet archived could be due that in a distributed environment it needs a little bit more time to transfer the log from the executor to configstore.

I tried to add a delay of 5 seconds before to check if the log exists. Need it find a better way to handle this.

[sgotti]

The error log delete error: Log for task XXXX in run YYYY is not yet archived could be due that in a distributed environment it needs a little bit more time to transfer the log from the executor to configstore.

Run fetching from the executor is always asyncronous:

agola/internal/services/runservice/scheduler.go

Lines 1109 to 1124 in 581a610

	func (s *Runservice) fetcherLoop(ctx context.Context) {
	for {
	log.Debugf("fetcher")
	if err := s.fetcher(ctx); err != nil {
	log.Errorf("err: %+v", err)
	}
	sleepCh := time.NewTimer(2 * time.Second).C
	select {
	case <-ctx.Done():
	return
	case <-sleepCh:
	}
	}
	}

Now fetching is executed every 2 seconds. The test fails for me also locally because it's not related to being in a single process or not since they always are distinct components.

You should wait for it for being archived but the run response provided by the gateway doesn't export information on the logs/archives fetch state (they are low level information that don't have to be exposed to external clients) so currently sleeping is the unique way in these integration tests.

[sgotti]

You should wait for it for being archived but the run response provided by the gateway doesn't export information on the logs/archives fetch state (they are low level information that don't have to be exposed to external clients)

Thinking more about this, if we want to show a Delete Log button in the ui, we should expose if the log is archived (and show the button only when archived). So the gateway api should expose something about the log state (LogArchived) in the RunTaskResponseStep (another PR)

[sgotti]

Now that #175 is merged we can use it in the tests to wait for the log to be archived.

[camandel]

Code modified as indicated and fixed the integration test (it checked logArchived only for step one).

[camandel]

For tests on logs with no existing step I used strings.Compare because the error message changes (runid , taskid and for get also the error prefix):

error getting log: log doesn't exist: no such step for task XXX in run YYY
error deleting log: no such step for task XXX in run YYY

I was wondering if it's possible to align the prefix log doesn't exist also for delete and if it's possible to check the error type (with util.IsNotExist) and not a substring of the error message. Thanks.

[sgotti]

For tests on logs with no existing step I used strings.Compare because the error message changes (runid , taskid and for get also the error prefix):

error getting log: log doesn't exist: no such step for task XXX in run YYY
error deleting log: no such step for task XXX in run YYY

was wondering if it's possible to align the prefix log doesn't exist also for delete

Yes, just use the same code used in LogsHandler to add the log doesn't exist::

agola/internal/services/runservice/api/api.go

Lines 196 to 201 in 519e7aa

	switch {
	case util.IsNotExist(err):
	httpError(w, util.NewErrNotExist(errors.Errorf("log doesn't exist: %w", err)))
	default:
	httpError(w, err)
	}

and if it's possible to check the error type (with util.IsNotExist) and not a substring of the error message.

You can use strings.HasPrefix which is cleaner than strings.Compare. Using only util.IsNotExits will check the error type but won't check the expected error text so it'll also catch an ErrNotExist caused by another code path.

[camandel]

Aligned error message prefix and changed check with strings.HasPrefix.

[sgotti]

@camandel Thanks a lot! Merging.