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

Fix/default values #10

Closed
wants to merge 41 commits into from
Closed

Conversation

lgallard
Copy link

@lgallard lgallard commented Sep 29, 2021

what

  • Change default values to include_map and exclude_map attributes
  • Fix shiled typo

why

  • To avoid different types in Terraform conditionals
  • Fix typo in naming resources.

references

@lgallard lgallard requested review from a team as code owners September 29, 2021 03:08
@lgallard
Copy link
Author

@Makeshift @dylanbannon we checked form side but we are not clear about what permission is needed by cloudpossebot in our repo. In fact the forked repo is public https://github.com/binbashar/terraform-aws-firewall-manager

This the error in your cicd pipeline:

remote: Permission to binbashar/terraform-aws-firewall-manager.git denied to cloudpossebot.
fatal: unable to access 'https://github.com/binbashar/terraform-aws-firewall-manager/': The requested URL returned error: 403

We introduced several fixes to make the module usable for all fms services, and would like to contribute by sharing those fixes with the community.

We will be more than pleased if you guide us to merge those changes into your module.

@mergify
Copy link

mergify bot commented Oct 6, 2021

This pull request is now in conflict. Could you fix it @lgallard? 🙏

lgallard and others added 9 commits October 6, 2021 15:04
* update logging configuration to use json functions (keeps same type of string) and use null not empty brace to allow non logging configs

* update contributors

* Auto Format

* update test files

* Auto Format

* update test files incluede region

* Auto Format

* pr comments - formatting and var name change

Co-authored-by: cloudpossebot <11232728+cloudpossebot@users.noreply.github.com>
* remove outputs

* Auto Format

Co-authored-by: cloudpossebot <11232728+cloudpossebot@users.noreply.github.com>
@lgallard
Copy link
Author

lgallard commented Oct 6, 2021

@Makeshift @dylanbannon we did a rebased to fix the conflicts due to your latest changes and hot-fixes, but still having issue with you CI/CD validation.

We checked form side but we are not clear about what permission is needed by cloudpossebot in our repo. In fact the forked repo is public https://github.com/binbashar/terraform-aws-firewall-manager

Can you help us with this in order to contribute to your module?

Thanks in advance!

cc: @exequielrafaela @diego-ojeda-binbash

@mergify
Copy link

mergify bot commented Nov 22, 2021

This pull request is now in conflict. Could you fix it @lgallard? 🙏

@nitrocode
Copy link
Member

@lgallard you'll have to run make init && make readme to regenerate the readme locally and then push up since the bot doesnt have access to the fork.

@Nuru
Copy link
Contributor

Nuru commented Nov 29, 2021

@lgallard wrote

We checked [from our] side but we are not clear about what permission is needed by cloudpossebot in our repo. In fact the forked repo is public https://github.com/binbashar/terraform-aws-firewall-manager

See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork for details about allowing us to modify your PR.

@lgallard
Copy link
Author

lgallard commented Nov 29, 2021

@Nuru @nitrocode conflicts were fixed. On the other hand I cound't find the option you pointed it out in here https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Please let us know if we need to do anything else to close the PR 🙏🏻

cc: @exequielrafaela @diego-ojeda-binbash

@nitrocode
Copy link
Member

@lgallard according to github, there are still conflicts. Could you look it over again ?

@Nuru
Copy link
Contributor

Nuru commented Nov 30, 2021

@lgallard Sorry about the difficulty getting this PR formatted and merged. Because we cannot push to your branch, we and our automated tools are not able to fix these issues for you.

You need to resolve the conflicts as explained by GitHub, and again run

make init
make pr/auto-format

and then commit and push the result.

Note: any conflicts in README.md or docs/terraform.md can be safely ignored by running

git add README.md docs/terraform.md

before running git commit and then running the make commands.

@Nuru Nuru mentioned this pull request Dec 1, 2021
@Nuru
Copy link
Contributor

Nuru commented Dec 1, 2021

@lgallard So that you do not have to do any more work, I am copying your changes to PR #17 and committing them there. I hope you don't mind, and if you do, please let me know what you would like me to do about it.

@Nuru Nuru closed this in #17 Dec 1, 2021
@lgallard
Copy link
Author

lgallard commented Dec 1, 2021

@Nuru thank you! That works for us. I hope next PRs we can find the permission issue on our end.

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.

Typo in variable and resource name
4 participants