Skip to content

Conversation

@emilhdiaz
Copy link

This PR adds support for iamManagedPolicies at the provider and function level.

I took the liberty to reorganize a small section of the code to make the createRoleForFunction method easier to follow.

Happy to make edits to the documentation and unit tests if you agree with the general approach here.

@coveralls
Copy link

coveralls commented Feb 21, 2019

Pull Request Test Coverage Report for Build 73

  • 31 of 36 (86.11%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.1%) to 90.541%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/lib/index.ts 31 36 86.11%
Totals Coverage Status
Change from base Build 71: -2.1%
Covered Lines: 119
Relevant Lines: 130

💛 - Coveralls

@glicht
Copy link
Member

glicht commented Feb 25, 2019

Thanks for contributing. Can you also add a small unit test to verify this feature.

@emilhdiaz
Copy link
Author

Sure thing. I'll add the unit tests this week.

@christophgysin
Copy link

@emilhdiaz Are you still working on this? I'm interested in the same feature, I might be able to finish this PR if needed.

@josephjohansson
Copy link

Again is this being worked on still? Would be awesome to have this feature!

@glicht
Copy link
Member

glicht commented Jun 7, 2019

It seems to be stuck. Open for contributions on this.

@emilhdiaz
Copy link
Author

My apologies, I'll try to pick this up again next week.

@ChristopheBougere
Copy link

Hi @emilhdiaz,
Any progress on this?
Happy to help if needed, I'd love to see this merged.

@emilhdiaz
Copy link
Author

@ChristopheBougere TBH haven't had a chance to get back to this. Would welcome the help to finally push this out.

@rfranco
Copy link

rfranco commented Nov 22, 2019

what is missing to merge it?

@Jamesargy6
Copy link

I would also love to see this feature!

@jaydg
Copy link

jaydg commented Feb 19, 2020

@ChristopheBougere TBH haven't had a chance to get back to this. Would welcome the help to finally push this out.

@emilhdiaz there is a MR against your MR wich adds the missing test. Could you please look into it?

AdrieanKhisbe pushed a commit to AdrieanKhisbe/serverless-iam-roles-per-function that referenced this pull request May 29, 2020
@AdrieanKhisbe
Copy link

Hello @glicht being really interested by the iamManagedPolicies support, I took care to refresh the stalled work from @emilhdiaz

I rebased it on top of master, solved conflict and added tests for the feature, here is a preview:

Would you prefer that I open another PR or do you have an alternative in mind?

Note that CI on latests serverless is broken, this is linked to this PR: https://github.com/serverless/serverless/pull/7722/files
But not sure you how to adapt ensure global role is present test correctly.

AdrieanKhisbe added a commit to CoorpAcademy/serverless-granular-iam that referenced this pull request Jun 8, 2020
…ies-and-godies ⚖️

Support for iam managed policies and goodies:
- Introduce support for `iamManagedPolicies`
- Contains reworked functionalone#19 with some extras (tests, duplication safeguard, prepare for git install)
@AdrieanKhisbe
Copy link

Hello @glicht and all,

Trying my chance again, I have a working version that handles iamManagedPolicies

Currently made a fully function fork there CoorpAcademy/serverless-granular-iam 🐙 + serverless-granular-iam 📦, and using it.
Though I think it's better to have changes incorporated into original code base and for being discontinued.

So, I'm just waiting for you to tell me how I should submit the changes (new pull request or else) ❔

Best regards
=)

@AdrieanKhisbe
Copy link

👋 Hello @glicht & @Enase

I reiterate my proposal to update the Pr, to make it work on serverless v2 now that the plugin has been recently adapted to support that.

As stated, CoorpAcademy/serverless-granular-iam 🐙 + serverless-granular-iam 📦 is not intended to be persistent, and it would be far better that the feature lands in the repo.

Please tell me what course of action you would prefer.
I can for instance rebase and adapt on current v3 and open a dedicated Pull request.

Best regards

@AdrieanKhisbe
Copy link

(ps: and I just saw #60 by @nl-brett-stime, didn't saw earlier as no notification received as @ handle was forgottent 🙈)

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.

10 participants