Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: file exporter path handling #2498

Conversation

hoangnv-bkhn
Copy link
Contributor

Description

  • Problem:

    • OutputDir in file exporter does not accept non-existent path.
    • If OutputDir is an empty string, the file path will become /{file-name}.json, which would cause it to be treated as an absolute path (instead of a relative path to the current working directory of the process).
      -> This will result in a permission denied error.
    • The documentation recommends that OutputDir should end with /.
      However, the current implementation makes the path look like {OutputDir}//{file-name}.json, resulting in a redundant /.
  • The way it was resolved:
    Check the validity of OutputDir and create if necessary

  • How to test the change:
    Configure OutputDir with cases: path does not exist, path with trailing /, path is empty string, and observe the results

Closes issue(s)

Resolve #2483

Checklist

  • I have tested this code
  • I have added unit test to cover this code
  • I have updated the documentation (README.md and /website/docs)
  • I have followed the contributing guide

Copy link

netlify bot commented Oct 11, 2024

Deploy Preview for go-feature-flag-doc-preview canceled.

Name Link
🔨 Latest commit 085aebf
🔍 Latest deploy log https://app.netlify.com/sites/go-feature-flag-doc-preview/deploys/670a4b46a318cd00083c8920

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 37.50000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 86.31%. Comparing base (ed39a6e) to head (bbe6500).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
exporter/fileexporter/exporter.go 37.50% 2 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2498      +/-   ##
==========================================
- Coverage   86.48%   86.31%   -0.18%     
==========================================
  Files         100      100              
  Lines        3544     3551       +7     
==========================================
  Hits         3065     3065              
- Misses        352      355       +3     
- Partials      127      131       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hoangnv-bkhn
Copy link
Contributor Author

To edit docs, besides changing the content of the .md files in /website/docs, what else do I need to do?
It seems that the docs on the website are not updated according to what I edited

Copy link
Owner

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoangnv-bkhn Thanks for your pull request, this makes the file exporter way better than it was before 🙇.
I have added a test in the case we have no outputDir.

Everything else is really great.
I will merge it soon and it will be available in the next release.


var filePath string
if outputDir == "" {
filePath = filename
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a test case for when we have no outputDir.

@thomaspoignant
Copy link
Owner

It seems that the docs on the website are not updated according to what I edited

Oh sorry I forgot to address this one.
The doc is versioned this is why you don't see the change, but if you add /next after /docs you are able to see the next version of the doc after the release (ex: https://gofeatureflag.org/docs/next).

In the preview for this PR it will be in https://deploy-preview-2498--go-feature-flag-doc-preview.netlify.app/docs/next

Copy link

sonarcloud bot commented Oct 12, 2024

@thomaspoignant thomaspoignant merged commit c1bab4f into thomaspoignant:main Oct 12, 2024
32 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(change) Improve Configuration of File Exporter
2 participants