Feat/act #408

ferencsarai · 2024-07-19T16:15:42Z

No description provided.

acha-bill

Nice! Could we also include tests to ensure that after updating the grantee list (whether adding or revoking access), the nodes either can or cannot download the file as expected?

Also, you should update https://github.com/ethersphere/bee/blob/master/.github/workflows/beekeeper.yml#L17 in the ACT PR to point to this beekeeper branch so that we see the tests in action.

acha-bill · 2024-07-23T08:19:32Z

pkg/bee/api/act.go

+	return a.client.requestData(ctx, http.MethodGet, "/"+apiVersion+"/grantee/"+addr.String(), nil, nil)
+}
+
+func (a *ActService) PatchGrantees(ctx context.Context, name string, data io.Reader, addr swarm.Address, haddr swarm.Address, batchID string) (ActGranteesResponse, error) {


name is not used

https://github.com/Solar-Punk-Ltd/beekeeper/blob/feat/act/pkg/bee/api/act.go#L56
name removed

martinconic · 2024-07-23T13:11:07Z

pkg/bee/api/api.go

-	if err = responseErrorHandler(r); err != nil {
-		return err
-	}
-
 	if v != nil && strings.Contains(r.Header.Get("Content-Type"), "application/json") {
 		_ = json.NewDecoder(r.Body).Decode(&v)


Should we use err = json.NewDecoder(r.Body).Decode(&v) and return if not nil , else we do the for and return nil , right?

this is done in responseErrorHandler

martinconic · 2024-07-23T13:19:33Z

pkg/bee/client.go

+	if err != nil {
+		return 0, nil, fmt.Errorf("download file %s: %w", a, err)
+	}
+	defer r.Close()


If an error occurs and r is not nil then this line will not be called and 'r' will not close.

'r' is of type io.ReadCloser. In case of an error, r will be nil, so Close should not be called to avoid a potential panic.

defer r.Close removed

https://github.com/Solar-Punk-Ltd/beekeeper/blob/feat/act/pkg/bee/client.go#L233

@ferencsarai sorry for misunderstanding, "defer r.Close()" should be there (like you placed it). I was trying to reply on @martinconic message, but I forgot to tag him. Please, revert like it was.

defer r.Close() added

martinconic · 2024-07-23T13:23:15Z

pkg/bee/client.go

+
+func (c *Client) AddActGrantees(ctx context.Context, f *File, o api.UploadOptions) (err error) {
+	h := fileHasher()
+	r, err := c.api.Act.AddGrantees(ctx, f.Name(), io.TeeReader(f.DataReader(), h), o)


should we have also some defer r.Close() ? Also in below similar situations?

r is ActGranteesResponse struct

martinconic · 2024-07-23T13:23:22Z

pkg/bee/client.go

@@ -750,6 +767,54 @@ func (c *Client) UploadFile(ctx context.Context, f *File, o api.UploadOptions) (
 	return
 }

+func (c *Client) UploadActFile(ctx context.Context, f *File, o api.UploadOptions) (err error) {
+	h := fileHasher()
+	r, err := c.api.Act.Upload(ctx, f.Name(), io.TeeReader(f.DataReader(), h), o)


should we have also some defer r.Close()

r is ActUploadResponse struct

gacevicljubisa · 2024-07-23T14:52:14Z

Nice! Could we also include tests to ensure that after updating the grantee list (whether adding or revoking access), the nodes either can or cannot download the file as expected?

Also, you should update https://github.com/ethersphere/bee/blob/master/.github/workflows/beekeeper.yml#L17 in the ACT PR to point to this beekeeper branch so that we see the tests in action.

Also in the same beekeeper.yaml (bee repoistory), if new "act" check should be used, in Integration tests new step should be added(something like this):

      - name: Test act
        id: act
        run: timeout ${TIMEOUT} beekeeper check --cluster-name local-dns --checks=ci-act

gacevicljubisa · 2024-07-23T15:08:16Z

pkg/check/act/act.go

+
+// Run executes act check
+func (c *Check) Run(ctx context.Context, cluster orchestration.Cluster, opts interface{}) (err error) {
+	c.logger.Info("ACT test started")


this log is redundant and can be removed, every check it is already printed here

beekeeper/cmd/beekeeper/cmd/check.go

Line 123 in b017aef

c.log.Infof("running check: %s", checkName)

removed

https://github.com/Solar-Punk-Ltd/beekeeper/blob/feat/act/pkg/check/act/act.go#L58

gacevicljubisa · 2024-07-23T15:33:28Z

pkg/check/act/act.go

+)
+
+// Options represents check options
+type Options struct {


options could be configurable in config.yaml and local.yaml, and then mapped in pkg/config/check.go where "act" is defined.

done

https://github.com/Solar-Punk-Ltd/beekeeper/blob/feat/act/config/config.yaml#L239
https://github.com/Solar-Punk-Ltd/beekeeper/blob/feat/act/config/local.yaml#L219
https://github.com/Solar-Punk-Ltd/beekeeper/blob/feat/act/pkg/config/check.go#L64

gacevicljubisa · 2024-07-23T15:36:09Z

pkg/bee/client.go

+	if err != nil {
+		return 0, nil, fmt.Errorf("download file %s: %w", a, err)
+	}
+	defer r.Close()


'r' is of type io.ReadCloser. In case of an error, r will be nil, so Close should not be called to avoid a potential panic.

gacevicljubisa · 2024-07-23T15:41:00Z

pkg/check/act/act.go

+		return fmt.Errorf("node %s: GetActGrantees after patch: addresses length is not 2", upNodeName)
+	}
+
+	c.logger.Info("ACT test completed")


also redundant log.Info

beekeeper/cmd/beekeeper/cmd/check.go

Line 142 in b017aef

c.log.Infof("%s check completed successfully", checkName)

removed
https://github.com/Solar-Punk-Ltd/beekeeper/blob/feat/act/pkg/check/act/act.go#L278

gacevicljubisa · 2024-07-23T15:45:20Z

pkg/check/act/act.go

+
+	c.logger.Info("ACT grantees added")
+
+	time.Sleep(3 * time.Second)


Why there is a sleep of 3 seconds, maybe some retry logic is needed as well?

without this GetActGrantees return 404

acha-bill · 2024-07-24T23:25:23Z

pkg/check/act/act.go

+		gPublisher, _ := swarm.ParseHexAddress(str)
+		history := file.HistroryAddress()
+		gSize, gHash, gPErr := upClient.DownloadActFile(ctx, file.Address(), &api.DownloadOptions{Act: &act, ActPublicKey: &gPublisher, ActHistoryAddress: &history, ActTimestamp: &timestamp})


Doesn't the file have only 1 publisher (the original uploader)?

Shouldn't this be testing that every grantee can download the file? i.e granteeClient.DownloadActFile

Test that revoked clients cannot download the file is missing

Yes, the file have only 1 publisher. Fixed
https://github.com/Solar-Punk-Ltd/beekeeper/blob/feat/act/pkg/check/act/act.go#L186

ferencsarai · 2024-07-31T18:46:59Z

test with bee:
https://github.com/Solar-Punk-Ltd/bee-beekeeper/actions/runs/10185687015/job/28175826202#step:12:1

fix: rearrage stake api (ethersphere#410)

ferencsarai closed this Jul 19, 2024

ferencsarai reopened this Jul 19, 2024

ferencsarai force-pushed the feat/act branch from 15ca2b4 to 1f18b1d Compare July 19, 2024 16:35

ferencsarai marked this pull request as ready for review July 22, 2024 08:51

istae requested review from acha-bill and martinconic July 22, 2024 12:23

acha-bill reviewed Jul 23, 2024

View reviewed changes

martinconic reviewed Jul 23, 2024

View reviewed changes

gacevicljubisa mentioned this pull request Jul 23, 2024

feat: introduce act ethersphere/bee#4692

Merged

4 tasks

gacevicljubisa reviewed Jul 23, 2024

View reviewed changes

istae requested a review from janos July 24, 2024 10:26

acha-bill reviewed Jul 24, 2024

View reviewed changes

ferencsarai force-pushed the feat/act branch from 8945442 to 4362255 Compare July 31, 2024 07:24

feat: act

32895eb

ferencsarai force-pushed the feat/act branch from 3dfe3de to 32895eb Compare July 31, 2024 18:19

feat: act resolve conflict

60e1c13

martinconic approved these changes Aug 2, 2024

View reviewed changes

Ferenc Sárai and others added 2 commits August 3, 2024 12:11

feat(act): add defer r.Close()

4536c06

Merge pull request #1 from ethersphere/master

3a88d2a

fix: rearrage stake api (ethersphere#410)

gacevicljubisa approved these changes Aug 4, 2024

View reviewed changes

acha-bill approved these changes Aug 4, 2024

View reviewed changes

istae merged commit 7fe190f into ethersphere:master Aug 6, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/act #408

Feat/act #408

ferencsarai commented Jul 19, 2024

acha-bill left a comment •

edited

Loading

acha-bill Jul 23, 2024

ferencsarai Jul 31, 2024

martinconic Jul 23, 2024

ferencsarai Jul 31, 2024

martinconic Jul 23, 2024

gacevicljubisa Jul 23, 2024

ferencsarai Jul 31, 2024

gacevicljubisa Aug 2, 2024 •

edited

Loading

ferencsarai Aug 3, 2024

martinconic Jul 23, 2024

ferencsarai Jul 31, 2024

martinconic Jul 23, 2024

ferencsarai Jul 31, 2024

gacevicljubisa commented Jul 23, 2024

gacevicljubisa Jul 23, 2024

ferencsarai Jul 31, 2024

gacevicljubisa Jul 23, 2024

ferencsarai Jul 31, 2024

gacevicljubisa Jul 23, 2024

gacevicljubisa Jul 23, 2024

ferencsarai Jul 31, 2024

gacevicljubisa Jul 23, 2024

ferencsarai Jul 31, 2024

acha-bill Jul 24, 2024

ferencsarai Jul 31, 2024

ferencsarai commented Jul 31, 2024


		c.logger.Info("ACT grantees added")

		time.Sleep(3 * time.Second)

Feat/act #408

Feat/act #408

Conversation

ferencsarai commented Jul 19, 2024

acha-bill left a comment • 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

Choose a reason for hiding this comment

gacevicljubisa Aug 2, 2024 • 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

gacevicljubisa commented Jul 23, 2024

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

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

ferencsarai commented Jul 31, 2024

acha-bill left a comment •

edited

Loading

gacevicljubisa Aug 2, 2024 •

edited

Loading