fix: support yaml in blob syncs #1522
Conversation
Signed-off-by: Michael Beemer <[email protected]>
dosubot
bot
added
the
size:L
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
| // TODO make consistent with the file sync | ||
| return buf.String(), nil | ||
| // return "", fmt.Errorf("filepath extension for URI: '%s' is not supported", hs.sync()) |
There was a problem hiding this comment.
This retains the current behavior but is inconsistent with the file sync implementation. I'd prefer to return an error if the file extension is invalid or undefined but it would be a breaking change.
Do not merge until a decision has been made on the expected behavior.
There was a problem hiding this comment.
I agree that throwing here would be a good idea. But we could also do a slow transition, and for now, only log an error and a deprecation warning that the current file is not supported, and in future versions, this will cause an error. wdyt?
There was a problem hiding this comment.
I think we should just add the break. We are still sub zero and I don't think it will effect many people, and the resolution is very clear.
Signed-off-by: Michael Beemer <[email protected]>
beeme1mr
changed the title
Signed-off-by: Michael Beemer <[email protected]>
| // TODO make consistent with the file sync | ||
| return buf.String(), nil | ||
| // return "", fmt.Errorf("filepath extension for URI: '%s' is not supported", hs.sync()) |
There was a problem hiding this comment.
I agree that throwing here would be a good idea. But we could also do a slow transition, and for now, only log an error and a deprecation warning that the current file is not supported, and in future versions, this will cause an error. wdyt?
| if hs.fileType == "" { | ||
| uriSplit := strings.Split(hs.Object, ".") | ||
| hs.fileType = uriSplit[len(uriSplit)-1] | ||
| } |
There was a problem hiding this comment.
It may be out of scope for this PR, but bringing it up for further/future consideration that we could also take into consideration the blob's content-type header. You would then have to decide which takes precedence when the header and extension contradict, so it would complicate things, but I'm imagining a case where I've uploaded a blob object with proper headers but with a generated key that doesn't have a file extension.
Not sure if any of the other syncs take that into account, so probably best not to introduce here unless/until it can be made consistent with other syncs (at least the ones where a content-type header can be passed).
So, anyways, just some food for thought, but, after all, it's not an unreasonable requirement for files to have extensions, just highlighting that there could be cases where you're consuming a blob that might, by default, not have a file extension generated.
(For example, I've encountered terraform modules in the wild that will default to storing blobs without extensions. Now, in my opinion, that is chaotic nonsense, but idk, some folks just want to watch the world burn, so what can you do 🤷)
There was a problem hiding this comment.
I agree on all counts, but I'm a bit confused (possibly just because we are using different terminology for the same thing). Are you referring to file MIME-types? As in, the result of this command on most linux systems:
file --mime-type flagd/config/samples/example_flags.json #/home/todd/git/flagd/config/samples/example_flags.json: application/jsonOr do you mean something do do with HTTP headers from blobs provided by cloud providers? If you mean the latter, I'm not sure how this would impact anything other than blob syncs. Perhaps the former could be generally useful though... it's probably better than file extensions, TBH.
There was a problem hiding this comment.
Yeah, i think that's a good idea. I'll see how easy it is to determine the content type from the blog storage. Unfortunately, it looks like YAML was only recently defined as a content type in IANA so it may be inconsistently implemented by cloud vendors.
There was a problem hiding this comment.
I agree on all counts, but I'm a bit confused (possibly just because we are using different terminology for the same thing). Are you referring to file MIME-types? [...]
Oh that's interesting, but definitely not thinking we'd sniff the files with file --mime-type.
Or do you mean something do do with HTTP headers from blobs provided by cloud providers? If you mean the latter, I'm not sure how this would impact anything other than blob syncs. Perhaps the former could be generally useful though... it's probably better than file extensions, TBH.
Rather, I was thinking about the existing Sync Configuration docs, which imply that I could serve any old file from any old web app by way of --uri https://.... (still not sure if grpc/k8s, i.e., --uri grpcs://.../--uri core.openfeature.dev://, would return anything like an HTTP Content-Type header, but if they do, then the more the merrier).
So, for example, I could imagine a scenario where it could be useful to serve flag definitions from a web app (which itself might be accessing blob storage, or dynamically constructing flag definitions from a database, etc.), and then start flagd with --uri https://my-web-app/some_flags, and have flagd merrily request and react based on the web app's response headers: Content-Type: application/yaml (accepted), Content-Type: application/json (accepted), or Content-Type: application/some-unknown-stuff (flagd fails to load this and logs an error, etc.).
If this all works, then flagd could even send out an accept header when making the sync requests, e.g., Accept: application/json, application/yaml, application/x-yaml, text/yaml, which would help with @beeme1mr's point about how recently YAML's official MIME type was defined.
Signed-off-by: Todd Baert <[email protected]>
This PR
Related Issues
Fixes #1512
How to test
make testormake test-core