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

Limit role name length #415

Merged
merged 3 commits into from
Jun 24, 2024
Merged

Limit role name length #415

merged 3 commits into from
Jun 24, 2024

Conversation

jsonar-cpapke
Copy link
Collaborator

Copy link
Collaborator

@eytannnaim eytannnaim left a comment

Choose a reason for hiding this comment

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

Does this do what you really need?
Shouldn't it be:

name = "${substr(var.name, 0, 64 - 5)}-role"

@lindanasredin
Copy link
Collaborator

You need to make this change in DAM and DRA as well to maintain the consistency.

@jsonar-cpapke
Copy link
Collaborator Author

@eytannnaim right, I meant to do length of -role to make it obvious what I was doing, but used the wrong data.

@jsonar-cpapke
Copy link
Collaborator Author

@lindanasredin I thought I only am supposed to manage Sonar code currently. I don't have any way currently to test anything in DAM or DRA.

@lindanasredin
Copy link
Collaborator

lindanasredin commented May 29, 2024

@jsonar-cpapke You are right basically, except for cases where we need to maintain consistency and it's a simple fix, not a big feature. Surely I don't expect you to test DAM and DRA. When you push the code the actions will test them. Sorry for being presumptuous in my previous message, this is the first time we face such a case and we need to talk about it. So if it's not too much trouble for you, can you please make the change for the other IAM roles? You can search "resource "aws_iam_role"" in the entire project. Actually now that I'm searching, I see there is one more location related to Sonar - the MSSQL.

@jsonar-cpapke
Copy link
Collaborator Author

Can we do that as a separate PR? This is currently preventing on of our QA from testing dsfkit.

@lindanasredin
Copy link
Collaborator

It won't matter because we have to release a new version of eDSF Kit for QA to get the fix, and we can't do it with a broken consistency, and we don't release on Thursday and we need to check if we have enough content to make a release.

Copy link
Collaborator

@jagdeep-sonar jagdeep-sonar left a comment

Choose a reason for hiding this comment

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

ok.

@jsonar-cpapke jsonar-cpapke merged commit 67f5181 into dev Jun 24, 2024
1 check passed
@jsonar-cpapke jsonar-cpapke deleted the cpapke/max-role-name-length branch June 24, 2024 23:03
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.

4 participants