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

lang/funcs: add "abspath" function #21409

Merged
merged 1 commit into from
Jul 2, 2019
Merged

Conversation

AndiDog
Copy link
Contributor

@AndiDog AndiDog commented May 23, 2019

Allows working around #21400, and covers a suggested function in #16697

@hashicorp-cla
Copy link

hashicorp-cla commented May 23, 2019

CLA assistant check
All committers have signed the CLA.

@mildwonkey
Copy link
Contributor

Hi @AndiDog, thank you for submitting this!

The test failure is due to the new abspath function missing from this test: https://github.com/hashicorp/terraform/blob/master/lang/functions_test.go
Can you add the function to that?

Now that I look at this error in travis, I will see about making the message clearer and more actionable.

@AndiDog
Copy link
Contributor Author

AndiDog commented May 24, 2019

The error is crystal clear, thanks. I simply didn't run a full make test because I had peaked at a similar commit and thought my newly added test would be enough to pass the complete suite. Adding the missing test in a moment.

Copy link
Member

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @AndiDog!

I left one minor comment inline, motivated by consistency with how the path. references behave in 0.12. If you don't have time to respond to that with a code change, please do let me know and we'll be happy to adjust it as part of merging, since it's a relatively small change and easy to test.

lang/funcs/filesystem.go Show resolved Hide resolved
@AndiDog
Copy link
Contributor Author

AndiDog commented May 28, 2019

@apparentlymart Ready for your review

@AndiDog
Copy link
Contributor Author

AndiDog commented Jun 26, 2019

Can we get this in? Still merges nicely to master and the change is very simple.

@peimanja
Copy link

peimanja commented Jul 2, 2019

Any update on this one?

@mildwonkey mildwonkey merged commit 042aead into hashicorp:master Jul 2, 2019
@mildwonkey
Copy link
Contributor

@AndiDog thanks for submitting this and addressing the feedback, I'll get this merged 🎉 🎉 🎉

@ghost
Copy link

ghost commented Aug 13, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Aug 13, 2019
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.

None yet

5 participants