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

Add a testcontainers_wiremock#RunContainerAndStopOnCleanup() method to simplify automatic termination #30

Merged

Conversation

Vayras
Copy link
Contributor

@Vayras Vayras commented Aug 9, 2023

As per the issue I have containerize the code the update code is in a new branch with same name as the issue

References

  • TODO

Submitter checklist

  • The PR request is well described and justified, including the body and the references
  • The PR title represents the desired changelog entry
  • The repository's code style is followed (see the contributing guide)
  • Test coverage that demonstrates that the change works as expected
  • For new features, there's necessary documentation in this pull request or in a subsequent PR to wiremock.org

Screenshot 2023-08-09 172259

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. I would actually expect not refactoring of a test case, but adding a new method in https://github.com/wiremock/wiremock-testcontainers-go/blob/main/tc-wiremock.go that makes the test more simple

It could be RunContainerAndStopOnCleanup() or so that takes the testing context as an extra argument

@Vayras
Copy link
Contributor Author

Vayras commented Aug 9, 2023

@oleg-nenashev thanks for your feedback , correct me if I'm wrong , the method needs to written in the tc-wiremock.go and used in the quickstart_test.go?

@oleg-nenashev
Copy link
Member

@Vayras yup, this is correct. See #21 for inspiration

@oleg-nenashev oleg-nenashev added the enhancement New feature or request label Aug 9, 2023
@Vayras
Copy link
Contributor Author

Vayras commented Aug 9, 2023

@oleg-nenashev should I implement these changes?

@oleg-nenashev
Copy link
Member

@Vayras this is an expectation in #27

@Vayras
Copy link
Contributor Author

Vayras commented Aug 9, 2023

@oleg-nenashev Okay , working on this

@Vayras
Copy link
Contributor Author

Vayras commented Aug 10, 2023

@oleg-nenashev I have commited and pushed some changes using terminal can you please let me know if they reflect and are updated?

Copy link
Member

@oleg-nenashev oleg-nenashev 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! It gets closer but we need to replace the /hello hardcoding by a proper mapping of parameters

tc-wiremock.go Outdated

func RunContainerAndStopOnCleanup(ctx context.Context, t *testing.T, mappingFileName string) (testcontainers.Container, error) {
container, err := RunContainer(ctx,
WithMappingFile("hello", mappingFileName),
Copy link
Member

Choose a reason for hiding this comment

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

That part needs to be parameterizable and, preferably, taking the same array as a RunContainer method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oleg-nenashev Thank you for your feedback , will make these changes

Copy link
Contributor Author

@Vayras Vayras Aug 15, 2023

Choose a reason for hiding this comment

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

Screenshot 2023-08-15 224903
I believe that by adding an 'Id' parameter, the issue will be resolved. I have made this change to the file

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

@Vayras I suggest keeping the original RunContainerSomething(ctx context.Context, opts ...testcontainers.ContainerCustomizer) interface so that the code is more flexible. You would need to just take this array as an argument and pass to the nested method

@Vayras
Copy link
Contributor Author

Vayras commented Aug 17, 2023

runcontainer
with this you can call the function like this :
container, err := RunContainerAndStopOnCleanup(ctx, t, []testcontainers.ContainerCustomizer{ WithMappingFile(id, mappingFileName), WithImage("customimage"), // Add other customizers })

@oleg-nenashev
Copy link
Member

Thanks! Let's see how the CI runs, and happy to integrate it afterwards

@oleg-nenashev oleg-nenashev changed the title testcontainter with auto termination Add a testcontainers_wiremock#RunContainerAndStopOnCleanup() method to simplify automatic termination Aug 17, 2023
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Thanks! I will update the README in the downstream PR, because there are a few other changes I wanted to land anyway

@oleg-nenashev oleg-nenashev merged commit 85753b7 into wiremock:main Aug 17, 2023
1 check passed
@Vayras
Copy link
Contributor Author

Vayras commented Aug 17, 2023

@oleg-nenashev It's truly been a great experience collaborating with you on this project. If the thought crosses your mind, I'd be honored to discuss the possibility of sponsorship for my work or even the potential of becoming a part of this organization. Please feel free to reach out anytime. I'm always eager to contribute my skills and expertise to address challenges as effectively as I can.

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Aug 17, 2023

@Vayras from what I understand, you're just starting with Golang. I might be open to discuss various open source internship options in the future, but at the same time I want to highlight that asking for being paid for your work in the future after doing a single newcomer friendly contribution is unlikely to be perceived well by maintainers. So I recommend to revise the message above if you send it to other projects

@Vayras
Copy link
Contributor Author

Vayras commented Aug 17, 2023

@oleg-nenashev Thank you for addressing this , finding an open source internship related to Golang would be very beneficial for me, and I think open source contribution , even with whatever little I do , keeps me motivated ,I'm fairly new to Golang , I have recently picked up learning this language any opportunity to get better at it is deeply appreciated.

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

Successfully merging this pull request may close these issues.

2 participants