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

doc fix: make file permission examples octal #1510

Merged
merged 2 commits into from
Aug 21, 2023

Conversation

martin-mfg
Copy link
Contributor

Examples of file permissions should be in octal format. But in the code examples they are not. This PR fixes that.

What does this PR do?

Change examples in docs.

Why is it important?

Docs should be correct.

Examples of file permissions should be in octal format. But in the code examples they are not.
@martin-mfg martin-mfg requested a review from a team as a code owner August 17, 2023 09:45
@netlify
Copy link

netlify bot commented Aug 17, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 85f47d3
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/64de620f59d45a000736fa99
😎 Deploy Preview https://deploy-preview-1510--testcontainers-go.netlify.app/features/copy_file
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -14,7 +14,7 @@ nginxC, err := GenericContainer(ctx, GenericContainerRequest{
Started: true,
})

nginxC.CopyFileToContainer(ctx, "./testdata/hello.sh", "/hello_copy.sh", 700)
nginxC.CopyFileToContainer(ctx, "./testdata/hello.sh", "/hello_copy.sh", 0700)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a linter for that provided by golangci but it usually suggest something like that

Suggested change
nginxC.CopyFileToContainer(ctx, "./testdata/hello.sh", "/hello_copy.sh", 0700)
nginxC.CopyFileToContainer(ctx, "./testdata/hello.sh", "/hello_copy.sh", 0o700)

If you find the linter it would worth adding it to golangci lint config with your PR , don't you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a newbie in golang, so maybe I'm wrong, but...
It seems golangci-lint doesn't have a check for octals: https://golangci-lint.run/usage/linters/
go-critic has such a check, but go-critic is not used in this repo: https://go-critic.com/overview.html#octalliteral

Do you want me to change the examle code to the suggested 0o... format anyway?

Copy link
Contributor

@mmorel-35 mmorel-35 Aug 17, 2023

Choose a reason for hiding this comment

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

Please follow gocritic advice. Gocritic can be added in a second time to golangci-lint config

@sonarcloud
Copy link

sonarcloud bot commented Aug 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

@martin-mfg thanks for this super tiny and valuable change in our docs!

Because we are in the transition plan to migrate the inline snippets to executable Go example tests, I'm merging this as is, although at some point we will eliminate any inline snippet in favour of examples.

Before #1498, I'd have asked you to look for any other file permission usage in the library, so we can update it to the octal format, but I noticed @mmorel-35 already added a lint step for it in that PR.

Therefore, LGTM!

@mdelapenya mdelapenya self-assigned this Aug 21, 2023
@mdelapenya mdelapenya added the documentation Docs, docs, docs. label Aug 21, 2023
@mdelapenya mdelapenya merged commit 7021689 into testcontainers:main Aug 21, 2023
7 checks passed
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Aug 21, 2023
* main:
  Golangci-lint for all go-modules (testcontainers#1498)
  doc fix: make file permission examples octal (testcontainers#1510)
  Add "new badge" in header (testcontainers#1512)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Docs, docs, docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants