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.

Sign up for GitHub

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

Jump to bottom

Updating module dependencies #53

Merged
merged 19 commits into from
Jul 2, 2020
Merged

Updating module dependencies #53

merged 19 commits into from
Jul 2, 2020

Conversation

jamengual
Copy link
Contributor

what

  • Updating all module dependencies

why

  • Improved module options
  • Added fix for github provider regresion

references

@jamengual
Copy link
Contributor Author

/test all

@jamengual
Copy link
Contributor Author

/test all

@jamengual
Copy link
Contributor Author

/rebuild-readme

@jamengual
Copy link
Contributor Author

/test all

@jamengual
Copy link
Contributor Author

/terraform fmt

@jamengual
Copy link
Contributor Author

/terraform-fmt

@jamengual
Copy link
Contributor Author

/test all

@nitrocode
Copy link
Member

Not sure why the tests are failing...

@nitrocode
Copy link
Member

/test all

nitrocode
nitrocode previously approved these changes Jul 1, 2020
View reviewed changes
@jhosteny
Copy link
Contributor

jhosteny commented Jul 1, 2020

It looks like nuke needs to be run before this can pass again. #52 is also a duplicate of this, which I will close once this passes.

Jose Amengual added 3 commits July 1, 2020 14:34
@jamengual
Copy link
Contributor Author

/test all

@jamengual
Copy link
Contributor Author

/test terratest

@jamengual
Copy link
Contributor Author

/test terratest

1 similar comment
@jamengual
Copy link
Contributor Author

/test terratest

@jamengual
Copy link
Contributor Author

/test all

@jamengual
Copy link
Contributor Author

/rebuild-readme

@jamengual
Copy link
Contributor Author

/test all

@jamengual jamengual requested review from a team, adamcrews, aknysh and jhosteny and removed request for a team July 2, 2020 05:53
brcnblc
brcnblc previously approved these changes Jul 2, 2020
View reviewed changes
Copy link

@brcnblc brcnblc left a comment

Choose a reason for hiding this comment

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

Change in variable names can break user code, Let's hope they use versioning properly

@jamengual
Copy link
Contributor Author

Change in variable names can break user code, Let's hope they use versioning properly

This was caused by an upstream module change, I'm not going to merge until we have a decision on that

@jhosteny
Copy link
Contributor

jhosteny commented Jul 2, 2020

Hi @jamengual, can you clarify what you are waiting on before merging?

@jamengual
Copy link
Contributor Author

Hi @jamengual, can you clarify what you are waiting for before merging?

There are upstream changes that forced this PR to change input values, I was hoping to revert back those changes since inadvertently are affecting this and other modules but I could merge this and open another PR, I'm open to suggestions

@jhosteny
Copy link
Contributor

jhosteny commented Jul 2, 2020

There are upstream changes that forced this PR to change input values, I was hoping to revert back those changes since inadvertently are affecting this and other modules but I could merge this and open another PR, I'm open to suggestions

Right, I didn't see what input values changed aside from the mount_points default type (which was the same at the prior module for container definitions)?

@jamengual
Copy link
Contributor Author

Right, I didn't see what input values changed aside from the mount_points default type (which was the same at the prior module for container definitions)?

https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/53/files#diff-02ad70b8a118d59ac14f9a8bdf408999R71

@jhosteny
Copy link
Contributor

jhosteny commented Jul 2, 2020

Ah, I saw the output change. I thought you meant there was an upstream variable change for one of the modules used by this module.

Personally, I would not mind updating my app when I changed the tag to reference ecr_repository_url. Though the name is different, the functionality and semantics are the same. It seems like that is the correct name anyway, and it is probably more intuitive to use the module if those match each other, and the AWS docs and terrform resource. But, if you wanted to avoid this, perhaps you can revert the output name reported by this module to ecr_registry_url (even though it gets ecr_repository_url from the referenced ECR module)?

Do the modules ever get a major version bump? I probably have not seen enough of them to know if that is done. I suppose that is the other consideration.

@jamengual
Copy link
Contributor Author

Ah, I saw the output change. I thought you meant there was an upstream variable change for one of the modules used by this module.

Personally, I would not mind updating my app when I changed the tag to reference ecr_repository_url. Though the name is different, the functionality and semantics are the same. It seems like that is the correct name anyway, and it is probably more intuitive to use the module if those match each other, and the AWS docs and terrform resource. But, if you wanted to avoid this, perhaps you can revert the output name reported by this module to ecr_registry_url (even though it gets ecr_repository_url from the referenced ECR module)?

Do the modules ever get a major version bump? I probably have not seen enough of them to know if that is done. I suppose that is the other consideration.

good point, will do

@jamengual
Copy link
Contributor Author

/test all

@nitrocode
Copy link
Member

/rebuild-readme

@nitrocode
Copy link
Member

/test all

osterman
osterman approved these changes Jul 2, 2020
View reviewed changes
Copy link
Member

@osterman osterman left a comment

Choose a reason for hiding this comment

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

Great job fixing the tests @jamengual !!

@jamengual jamengual merged commit 60272b4 into master Jul 2, 2020
@jamengual jamengual deleted the updating_module_dependencies branch July 2, 2020 22:09
@osterman
Copy link
Member

osterman commented Jul 3, 2020

Do the modules ever get a major version bump? I probably have not seen enough of them to know if that is done. I suppose that is the other consideration.

@jhosteny we're deliberately sticking with 0.x

See: https://docs.cloudposse.com/community/faq/#when-do-we-cut-new-releases

@osterman
Copy link
Member

osterman commented Jul 3, 2020

Change in variable names can break user code, Let's hope they use versioning properly

@brcnblc since we're 0.x we are permitted to change anything at anytime =) while we try to maintain stability, interfaces are not yet stable in all cases.

@jhosteny jhosteny mentioned this pull request Jul 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rb-nr rb-nr rb-nr approved these changes

@osterman osterman osterman approved these changes

@brcnblc brcnblc brcnblc left review comments

@nitrocode nitrocode nitrocode left review comments

@adamcrews adamcrews Awaiting requested review from adamcrews

@aknysh aknysh Awaiting requested review from aknysh

@jhosteny jhosteny Awaiting requested review from jhosteny

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jamengual @nitrocode @jhosteny @osterman @brcnblc @rb-nr @actions-bot