Skip to content

Conversation

@thomaspoignant
Copy link
Member

@thomaspoignant thomaspoignant commented Jul 18, 2022

This is a 1st version of the go-feature-flag provider.

@thomaspoignant thomaspoignant changed the title Go feature flag provider feat: Go feature flag provider Jul 19, 2022
Signed-off-by: Thomas Poignant <[email protected]>
Signed-off-by: Thomas Poignant <[email protected]>
@thomaspoignant
Copy link
Member Author

@beeme1mr I struggle with the dependencies and where to store them in the mono repo.
I have put them in the package.json close to the provider but it is not working for the tests (see the CI workflow falling).

Should I place them on the top level instead?
Is this something we have to think about for this repo?

@beeme1mr
Copy link
Member

Hey @thomaspoignant, you should put dependencies at the root of the project. NX will automatically detect and include runtime dependencies as peer dependencies when it builds your package.

@beeme1mr
Copy link
Member

I'll take a look at your branch and see if I can get the test passing.

@beeme1mr beeme1mr force-pushed the go-feature-flag-provider branch from 42c76a2 to daea160 Compare July 19, 2022 19:16
@beeme1mr
Copy link
Member

Hey @thomaspoignant, I was able to get it working. I tried to make my changes in granular commits so you could more easily follow my changes. There's a bit of NX magic involved but it's not too bad. Let me know if you have any questions.

@thomaspoignant
Copy link
Member Author

Thanks a lot @beeme1mr for your help 🙏
I was not used to NX that's the main reason why I implemented bad patterns (such as the dependencies at the wrong level).
I think the PR is now Ready for review.

I saw your commits and I wonder if some things should not be part of the npm run generate-provider command?
I am referring to b54196f and 15b2e8a.
This looks like something that will be required for all providers, so I wonder if we should not add this to the templates?

@thomaspoignant thomaspoignant marked this pull request as ready for review July 20, 2022 07:11
@beeme1mr
Copy link
Member

Hey @thomaspoignant, I'll try and review this today. I'll also create an issue to document how dependencies should be managed in NX.

15b2e8a should already be included in the generator. You can see the template here. I just tested it and it seem to be working as expected. Not sure what happened in your case.

Signed-off-by: Michael Beemer <[email protected]>
@beeme1mr
Copy link
Member

18c11da was added to the generator after you ran it. Future providers and hooks will have this setup automatically.

Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for being the first official OpenFeature node js provider!

@toddbaert
Copy link
Member

@thomaspoignant I updated the generator with https://github.com/open-feature/node-sdk-contrib/pull/63/files

It adds some additional scripts to the package.json to enable releasing. I will add this to this PR if you dont mind to make sure all the release process works as expected.

@toddbaert toddbaert force-pushed the go-feature-flag-provider branch from b1b88f9 to accd42f Compare July 21, 2022 18:55
@toddbaert
Copy link
Member

@thomaspoignant this is done. You can view my changes on this commit

This stuff is now included in the generator, my commit just backports it to your changes.

@toddbaert toddbaert merged commit f32d3b0 into open-feature:main Jul 21, 2022
@github-actions github-actions bot mentioned this pull request Jul 21, 2022
@thomaspoignant thomaspoignant deleted the go-feature-flag-provider branch July 21, 2022 19:10
@thomaspoignant
Copy link
Member Author

@toddbaert thanks a lot for your changes.
It seems that the deployment failed https://github.com/open-feature/node-sdk-contrib/runs/7456249885?check_suite_focus=true

I can have a look tomorrow when I will be in front of my computer

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.

3 participants