feat: Implement GSP-700 Config Features and DefaultPairs via Connection String #699

JinnyYi · 2021-07-30T12:02:34Z

codecov · 2021-07-30T12:03:07Z

Codecov Report

Merging #699 (b0546b8) into master (f14e890) will decrease coverage by 0.20%.
The diff coverage is 4.91%.

@@            Coverage Diff             @@
##           master     #699      +/-   ##
==========================================
- Coverage   12.35%   12.14%   -0.21%     
==========================================
  Files          22       22              
  Lines        1441     1490      +49     
==========================================
+ Hits          178      181       +3     
- Misses       1256     1302      +46     
  Partials        7        7

Flag	Coverage Δ
unittests	`12.14% <4.91%> (-0.21%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tests/generated.go	`1.77% <4.91%> (+0.34%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f14e890...b0546b8. Read the comment docs.

Xuanwo · 2021-07-30T12:06:07Z

Please start GSP first before implement this feature, we could reject huge changes without already approved GSP.

Xuanwo · 2021-08-12T06:16:52Z

tests/connstr_test.go

+			},
+			nil,
+		},
+		//{


Do we need this test?

Xuanwo · 2021-08-12T06:25:03Z

tests/generated.go

@@ -591,8 +681,36 @@ func parsePairStorageNew(opts []Pair) (pairStorageNew, error) {
 			}
 			result.HasWorkDir = true
 			result.WorkDir = v.Value.(string)
+		// Default pairs
+		case "default_content_type":
+			defaultPairs = true


Maybe we can simplify the logic via respect to the order of pairs?

We don't need to take default_storage_pairs first any more, instead we always take the order.

If user input WithDefaultStoragePairs and WithDefaultStorageClass, we will take WithDefaultStoragePairs first.

If user input WithDefaultStorageClass and WithDefaultStoragePairs, we will take WithDefaultStorageClass.

After this change, we don't need to maintain defaultPairs or enableFeatures, we can set HasStorageFeatures directly.

NOTICE: this change requires an update of GSP.

Does this mean that we do not consider the case where there is no intersection，like:

oss.WithDefaultStorageClass("Standard") // ... oss.WithDefaultStoragePairs(oss.DefaultStoragePairs{Write: []types.Pair{ps.WithContentType("application/text")}})

Yes, we can use them concurrently, they will not affect each other.

Maybe we need to keep these two variables defaultPairs and enableFeatures as when there is more than one default pairs(DefaultStorageClass and DefaultContentType, or EnableVirtualDir and EnableVirtualLink), we can't set HasDefault*Pairs or Has*Features to true every time, otherwise we would ignore some default values？

DefaultStorageClass and DefaultContentType will always be appened into DefaultStorageParis, I think setting HasDefaultStorageParis is the same with defaultPairs.

JinnyYi · 2021-08-13T08:41:25Z

cmd/definitions/tmpl/service.tmpl

@@ -103,7 +103,7 @@ func setStorageSystemMetadata(s *StorageMeta, sm StorageSystemMetadata) {
 }

 {{- range $_, $v := .Pairs }}
-{{- if not $v.Global }}
+{{- if and (not $v.Global) (not (hasPrefix $v.Name "enable_")) }}


I removed WithEnableXxx() for features as we can use <type>.WithStorageFeatures(<type>.StorageFeatures{Xxx: true, }) during initialize. I'm not sure whether it's necessary.

I think it's OK to allow users to enable a feature via WithEableXxx().

OK, I'll roll back this change.

For bool types, maybe we can generate WithEnableXxxx() directly? With the internal value set to true.

JinnyYi · 2021-08-13T08:47:30Z

tests/generated.go

-				continue
-			}
-			enableFeatures = true
+			result.HasStorageFeatures = true
 			result.StorageFeatures.LoosePair = v.Value.(bool)


I found the last one will be picked if there are multiple enable_loose_pair.

How about generating HasEnableLoose to prevent it?

Maybe treat enable_loose as a normal pair can prevent those strange edge cases?

JinnyYi · 2021-08-17T02:34:52Z

services/new.go

+			if v != "" {
+				opt = append(opt, v, "true")
+			} else {
+				// && , ignore
+				continue
+			}


It's hard-coded here.
How about not generate pairs for enable_xxx features and register parsing functions for StorageFeatures and ServiceFeatures? Then we can collection pairs without values for StorageFeatures or ServiceFeatures parsing?

Current implementation LGTM, we will refactor this part later, so don't be worried.

Xuanwo

I have some questions and let's discuss in https://matrix.to/#/#beyondstorage@gsp-700:matrix.org

Xuanwo · 2021-08-17T08:11:51Z

cmd/definitions/tmpl/service.tmpl

+    {{- end }}
+    {{- end }}
+    {{- if $needPair }}
+    "github.com/beyondstorage/go-storage/v4/pairs"


How about always import pairs? Like:

import "github.com/beyondstorage/go-storage/v4/pairs" var _ pairs.XXXX

Xuanwo · 2021-08-17T08:14:05Z

cmd/definitions/tmpl/service.tmpl

@@ -103,7 +103,7 @@ func setStorageSystemMetadata(s *StorageMeta, sm StorageSystemMetadata) {
 }

 {{- range $_, $v := .Pairs }}
-{{- if not $v.Global }}
+{{- if and (not $v.Global) (not (hasPrefix $v.Name "enable_")) }}


For bool types, maybe we can generate WithEnableXxxx() directly? With the internal value set to true.

Xuanwo · 2021-08-17T08:45:53Z

The PR is too big, I updated steps in #704, let's split it into PRs.

Xuanwo · 2021-08-25T02:12:34Z

Maybe we can close this PR for now?

Support config features and defaultPairs

930083d

Xuanwo mentioned this pull request Aug 2, 2021

Weekly report since 2021-07-26 beyondstorage/community#28

Closed

JinnyYi mentioned this pull request Aug 4, 2021

GSP-700: Config Features and DefaultPairs via Connetion String #700

Merged

JinnyYi added 3 commits August 9, 2021 22:29

Update implementation

be26e75

Update default pairs

a854930

Merge master into default-configs

f31304a

JinnyYi marked this pull request as ready for review August 10, 2021 11:45

JinnyYi changed the title ~~Support config features and defaultPairs~~ cmd/definitions: Implement GSP-700 Config Features and DefaultPairs via Connection String Aug 12, 2021

Xuanwo changed the title ~~cmd/definitions: Implement GSP-700 Config Features and DefaultPairs via Connection String~~ feat: Implement GSP-700 Config Features and DefaultPairs via Connection String Aug 12, 2021

Xuanwo reviewed Aug 12, 2021

View reviewed changes

JinnyYi added 2 commits August 12, 2021 20:25

Merge master into default-configs

96b0e4e

Update handle conflict

bcc84d0

JinnyYi commented Aug 13, 2021

View reviewed changes

Update format for features

b0546b8

JinnyYi commented Aug 17, 2021

View reviewed changes

Xuanwo reviewed Aug 17, 2021

View reviewed changes

JinnyYi closed this Aug 25, 2021

Xuanwo deleted the default-configs branch August 25, 2021 03:36

Xuanwo mentioned this pull request Aug 30, 2021

Weekly report since 2021-08-23 beyondstorage/community#43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement GSP-700 Config Features and DefaultPairs via Connection String #699

feat: Implement GSP-700 Config Features and DefaultPairs via Connection String #699

JinnyYi commented Jul 30, 2021 •

edited

Loading

codecov bot commented Jul 30, 2021 •

edited

Loading

Xuanwo commented Jul 30, 2021 •

edited

Loading

Xuanwo Aug 12, 2021

Xuanwo Aug 12, 2021

JinnyYi Aug 12, 2021 •

edited

Loading

Xuanwo Aug 12, 2021

JinnyYi Aug 12, 2021

Xuanwo Aug 13, 2021

JinnyYi Aug 13, 2021

Xuanwo Aug 13, 2021

JinnyYi Aug 13, 2021

Xuanwo Aug 17, 2021

JinnyYi Aug 13, 2021

Xuanwo Aug 16, 2021

JinnyYi Aug 17, 2021

Xuanwo Aug 17, 2021

JinnyYi Aug 17, 2021

Xuanwo left a comment

Xuanwo Aug 17, 2021

Xuanwo Aug 17, 2021

Xuanwo commented Aug 17, 2021

Xuanwo commented Aug 25, 2021

feat: Implement GSP-700 Config Features and DefaultPairs via Connection String #699

feat: Implement GSP-700 Config Features and DefaultPairs via Connection String #699

Conversation

JinnyYi commented Jul 30, 2021 • edited Loading

codecov bot commented Jul 30, 2021 • edited Loading

Codecov Report

Xuanwo commented Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinnyYi Aug 12, 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

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

Xuanwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xuanwo commented Aug 17, 2021

Xuanwo commented Aug 25, 2021

JinnyYi commented Jul 30, 2021 •

edited

Loading

codecov bot commented Jul 30, 2021 •

edited

Loading

Xuanwo commented Jul 30, 2021 •

edited

Loading

JinnyYi Aug 12, 2021 •

edited

Loading