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

azurite preset based on the localstack preset #837

Merged
merged 19 commits into from Jul 29, 2023

Conversation

aeolus3000
Copy link
Contributor

@aeolus3000 aeolus3000 commented Mar 11, 2023

Proposing a azurite preset.

I would still need help with the following steps:
Generate [client SDK code]

Azurrite offers other stuff like table store but I didn't need that yet. Does anyone feel energetic to add that here?

@orlangure
Copy link
Owner

This is fantastic, thank you for the contribution!

I wasn't aware of azurite, it looks very useful, I hope it gets extended with more azure services in the future 😼

I wasn't able to properly review the code, but I have 2 small comments so far:

  1. No need to generate the SDK, I stopped supporting it since it consumes way to much energy for very little to no value. All your gnomock server and swagger changes are enough.
  2. Please add the new preset tests (preset and server tests) to github actions and circleci configuration files, otherwise these tests won't run automatically. If you don't have time to do that, let me know, and I'll add these myself.

I will find time to go over the code and provide more feedback if needed. So far it looks great and very similar to localstack, which is very nice.

@aeolus3000
Copy link
Contributor Author

Thanks for having a look into it and the missing files!

They are added now.

@aeolus3000
Copy link
Contributor Author

I think that the changes in CONTRIBUTING.md can be reverted.

Correct, @orlangure ?

Copy link
Owner

@orlangure orlangure left a comment

Choose a reason for hiding this comment

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

This is so cool, thank you!

A couple of small comments I'd be happy you address, and this is good to go.

Also, can you please rebase from master? There was another preset merged just now, and there are a few conflicts because of it. Thanks!

CONTRIBUTING.md Outdated
Comment on lines 87 to 95
1. Generate [client SDK
code](https://github.com/orlangure/gnomock#using-gnomock-server). Gnomock
uses OpenAPI to generate client code from swagger specification.

1. Add client SDK tests for the new preset. These tests go into
[`sdktest`](https://github.com/orlangure/gnomock/tree/master/sdktest)
package. They allow to make sure that client code in languages other than Go
does not break after each update.

Copy link
Owner

Choose a reason for hiding this comment

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

These are no longer required, I removed these points a few days ago😼

@@ -0,0 +1,73 @@
# Gnomock Blobstorage
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure what's the best name for this one.

In localstack, we call it "localstack", and there are internal services that it exposes. Even though the users look for s3 or sqs, the preset is still named localstack.

I think we should keep this convention, and since you already named the preset azurite, I believe the readme should also use the term "azurite".

The Example (this is great by the way!) on the other hand should keep the name "blog storage" in it, because it demonstrates a subset of azurite capabilities.

Do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the test i'm fine.

Or do you refer to the internal/gnomockd/azurite_test.go?
In this one I would leave it as "TestAzurite" because it is the same in localstack (it only tests s3 but is named localstack)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please check once more?

preset/azurite/README.md Outdated Show resolved Hide resolved
Copy link
Owner

@orlangure orlangure left a comment

Choose a reason for hiding this comment

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

Thank you for your great work and sorry for the delay in getting this merged.
A few more minor comments for you to address if that's ok.
Also, it appears that the tests are failing, and there are some lint warnings. Will you have time to resolve these?

go.mod Outdated
Comment on lines 33 to 35
k8s.io/api v0.26.1
k8s.io/apimachinery v0.26.1
k8s.io/client-go v0.26.1
Copy link
Owner

Choose a reason for hiding this comment

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

Some of the dependencies were accidentally rolled back, can you revert please?

preset/azurite/preset_test.go Outdated Show resolved Hide resolved
preset/azurite/preset_test.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 12, 2023

Codecov Report

Patch coverage: 84.81% and project coverage change: -0.37 ⚠️

Comparison is base (97f7ed9) 85.87% compared to head (a0ee700) 85.50%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #837      +/-   ##
==========================================
- Coverage   85.87%   85.50%   -0.37%     
==========================================
  Files          50       55       +5     
  Lines        2350     2567     +217     
==========================================
+ Hits         2018     2195     +177     
- Misses        173      193      +20     
- Partials      159      179      +20     
Impacted Files Coverage Δ
preset/azurite/options_blobstorage.go 70.14% <70.14%> (ø)
preset/vault/preset.go 77.41% <77.41%> (ø)
preset/k3s/preset.go 80.39% <94.11%> (-0.95%) ⬇️
preset/azurite/options.go 100.00% <100.00%> (ø)
preset/azurite/preset.go 100.00% <100.00%> (ø)
preset/cassandra/preset.go 93.10% <100.00%> (ø)
preset/cockroachdb/preset.go 81.53% <100.00%> (ø)
preset/elastic/preset.go 68.70% <100.00%> (+0.24%) ⬆️
preset/k3s/options.go 100.00% <100.00%> (ø)
preset/mariadb/preset.go 88.57% <100.00%> (ø)
... and 8 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@aeolus3000
Copy link
Contributor Author

@orlangure can you please cross check again?
I don't understand why the test coverage is so low. when I run the tests locally I have more than 85 % coverage. Am I missing something?

@orlangure
Copy link
Owner

Hey @aeolus3000, your tests and their coverage are great, the build just didn't start automatically because you still don't have any merged contributions (not for long 😼). Now that I manually approved those, codecov no longer complains, but you do have a couple of minor lint warnings to resolve. After that this PR will be merged, and I'll prepare a new release so everybody can enjoy your work. Thank you!

@aeolus3000
Copy link
Contributor Author

@orlangure I have found some time on the weekend to work on the lint checks. Can you please approve the workflows?

Copy link
Owner

@orlangure orlangure left a comment

Choose a reason for hiding this comment

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

Amazing work, thank you for doing this!

@orlangure
Copy link
Owner

Would you mind resolving merge conflicts so I can merge this?

@aeolus3000
Copy link
Contributor Author

Hey @orlangure

I can't find a merge conflict. Can you point that out to me please?

@orlangure orlangure merged commit 5ed38a4 into orlangure:master Jul 29, 2023
26 checks passed
@orlangure
Copy link
Owner

Hey @aeolus3000,
I wasn't able to merge this from GitHub UI, but mobile version didn't show any warnings about conflicts in code. Sorry for the confusion, fully merged now. This is a great addition, thank you for your work and patience 😼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants