Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Credstash grant #266

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Credstash grant #266

wants to merge 3 commits into from

Conversation

Magicloud
Copy link
Contributor

Passed mock data planning. Fixes #265.

…context with `map`.

The original way of creating map at runtime from two lists are easy to go wrong, and acturally wrong. It went through the lists by role count, which does not make sense.
…context with `map`.

The original way of creating map at runtime from two lists are easy to go wrong, and acturally wrong. It went through the lists by role count, which does not make sense.
@Magicloud Magicloud requested a review from ketzacoatl October 11, 2019 14:29
Copy link
Contributor

@qrilka qrilka left a comment

Choose a reason for hiding this comment

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

LGTM

@ketzacoatl
Copy link
Contributor

Should we also drop role_count and refactor/cleanup those variables all the way?

cc @Magicloud / @qrilka

@qrilka
Copy link
Contributor

qrilka commented Dec 6, 2019

@ketzacoatl yes, that was one of my proposals, just put it into a list of objects with names and ARNs. The current approach is quite awkward and error prone

@Magicloud
Copy link
Contributor Author

I am with removing "role_count". But seeing that comment, "circumvent issue", the issue is still open (for a few years....). @ketzacoatl Is that issue fixed or unrelated now, especially we are on 0.12 now.

@ketzacoatl
Copy link
Contributor

@Magicloud what are next steps for this PR?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

credstash-grant-reader could not be planned with empty(default) context_keys and context_values
3 participants