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

Adding support for terraform provider registry #50

Closed
wants to merge 22 commits into from

Conversation

sponte
Copy link
Contributor

@sponte sponte commented Dec 15, 2020

Thanks for creating citizen - a missing registry for Terraform Modules. I have a need to host custom providers and thought of extending your project.

This PR adds support for hosting custom providers utilising Hashicorp's provider registry protocol documented at https://www.terraform.io/docs/internals/provider-registry-protocol.html.

Changes

  1. Added new endpoint for providers
  2. Added new endpoint for publishers
  3. Added citizen cli commands for publishing providers and trusted publishers
  4. Added unit/integration tests that cover new functionality
  5. Updated README to reflect the changes

I have updated the version to 1.0.0 to indicate breaking changes:

  1. citizen publish command is no longer top level command as citizen can now publish modules, providers and publishers
  2. Internals: Changed storage paths for modules to accommodate providers, now the root of the storage folder will contain top level modules and providers directories. This means new version won't work with existing registries, although careful migration is possible

lib/server.js Outdated
@@ -1,4 +1,4 @@
const http = require('http');
const https = require('https');
Copy link

Choose a reason for hiding this comment

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

It looks like out-of-scope changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, will probably remove it but terraform provider registry requires HTTPS - once I've sorted the tests out, I should be able to remove https and https-localhost

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 have now removed https-localhost dependency as I added tests that utilise ngrok

@sponte sponte changed the title [WIP] Initial work on supporting terraform provider registry Initial work on supporting terraform provider registry Dec 21, 2020
@sponte sponte changed the title Initial work on supporting terraform provider registry Adding support for terraform provider registry Dec 21, 2020
@sponte
Copy link
Contributor Author

sponte commented Dec 21, 2020

Ok @ad-m - this is ready to review now - please have a look and let me know if you need me to make any changes before merging.

@sponte
Copy link
Contributor Author

sponte commented Dec 29, 2020

Not sure who will be reviewing this @outsideris - I referenced @ad-m earlier as he commented on the commit before but since you approved my other PRs (HCLv2/Terraform v12 and small update to the docs), I thought I'd bring this one to your attention. Please let me know your thoughts.

@ad-m
Copy link

ad-m commented Dec 30, 2020

For clarity, I have no relationship with @outsideris . I am part of the community of this project because I follow it. I never even ddownloaded and started it.

@outsideris
Copy link
Owner

Sorry for late response.
I will check your contribution soon.
Thank you.

@outsideris outsideris mentioned this pull request Feb 11, 2021
Copy link
Owner

@outsideris outsideris 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 contribution. Now we support providers!!!

Because I refactored code base on master after this PR, I merged this PR manually on my local. And then I will test and modify citizen intensively. (So, you don't need to fix this PR.)

.env
test/integration/fixture/provider
coverage/
.nyc_output/
Copy link
Owner

Choose a reason for hiding this comment

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

I will clean up gitignore.

@@ -19,11 +19,12 @@ const normalizePort = (val) => {
return false;
};

const run = () => {
const run = async () => {
Copy link
Owner

Choose a reason for hiding this comment

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

we don't need async here.

@@ -1,6 +1,6 @@
{
"name": "citizen",
"version": "0.3.3",
"version": "1.0.0",
Copy link
Owner

Choose a reason for hiding this comment

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

I don't want to version up now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason version went up is that this MR introduces breaking changes in citizen cli

// error.status = 409;
// error.message = `${destPath} is already exist.`;
// return next(error);
// }
Copy link
Owner

Choose a reason for hiding this comment

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

unnecessary comments

@outsideris
Copy link
Owner

I rebased this PR on my local, see d698abf

@outsideris outsideris closed this Feb 11, 2021
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