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

chore: major cleanup #6

Merged
merged 8 commits into from
May 11, 2024
Merged

Conversation

shanduur
Copy link

Describe your changes

Based on the commit messages provided, here is a description of the changes made:

  1. Reworking Dockerfile to use multistage build:

    • The Dockerfile has been reworked to utilize a multistage build approach.
    • The build process now uses separate images for the build stage and the runtime stage.
  2. Cleanup and update tooling:

    • The Makefile has been cleaned up, removing unused or outdated variables and task.
    • The pre-commit hooks have been updated to ensure code quality and adherence to coding standards.
  3. Reducing dependencies and cleaning up code:

    • The dependency on the Cobra and Viper libraries has been removed.
    • Parsing of environment variables has been added, which is a replacement for the removed dependencies.
  4. Extending documentation for servers:

    • The godoc documentation has been extended for both the Provisioner and Identity servers.
    • This may include additional explanations, comments, and examples to help other developers understand and use these servers more effectively.
  5. Standardizing labels on container:

    • The commit involves adding labels that conform to the org.opencontainers.image.* standard.
    • These labels include metadata about the container image, such as its name, version, and other important information, and help maintain consistency and compatibility with various tools and platforms.

Issue ticket number and link

N/A

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • CHANGELOG.md has been updated should there be relevant changes in this PR.

@shanduur
Copy link
Author

I hope I didn't broke any CI, but I tried to maintain all defaults and behaviour of both the driver and Makefile.

Signed-off-by: Mateusz Urbanek <[email protected]>
@jecluis jecluis self-assigned this Apr 28, 2024
@jecluis
Copy link

jecluis commented Apr 28, 2024

Hi @shanduur - thank you for the submission!

I'll review it in the coming days, if not earlier. :)

Copy link

@jecluis jecluis left a comment

Choose a reason for hiding this comment

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

Other than a typo in the Dockerfile, and a few non-blocking nits, this looks absolutely amazing work.

Thank you so much for all the effort you put into it, and sincere apologies for the delay in reviewing. :)

Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
cmd/s3gw-cosi-driver/main.go Outdated Show resolved Hide resolved
pkg/driver/driver.go Outdated Show resolved Hide resolved
pkg/driver/identityserver.go Outdated Show resolved Hide resolved
pkg/driver/provisioner.go Outdated Show resolved Hide resolved
pkg/driver/provisioner_test.go Outdated Show resolved Hide resolved
pkg/envflag/envflag.go Outdated Show resolved Hide resolved
pkg/envflag/envflag_test.go Outdated Show resolved Hide resolved
@shanduur shanduur requested a review from jecluis May 11, 2024 08:45
jecluis
jecluis previously approved these changes May 11, 2024
@jecluis
Copy link

jecluis commented May 11, 2024

@shanduur lint failed for some reason - mind checking whether this looks like it's coming from your changes?

https://github.com/s3gw-tech/s3gw-cosi-driver/actions/runs/9042470387/job/24849064117?pr=6#step:4:123

Let me know if not and I'll try figuring it out.

Signed-off-by: Mateusz Urbanek <[email protected]>
@jecluis jecluis merged commit 132c710 into s3gw-tech:s3gw May 11, 2024
1 check passed
@jecluis
Copy link

jecluis commented May 11, 2024

@shanduur merged - thank you for your patience and effort! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants