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

Allow "[profile default]" in .aws/config #484

Merged
merged 3 commits into from
Oct 15, 2021

Conversation

c42f
Copy link
Contributor

@c42f c42f commented Oct 8, 2021

The awscli allows either a section [default] or [profile default] in
the aws config file.

I had [profile default] in my config and was surprised when this worked with awscli but not with AWS.jl.

@omus
Copy link
Member

omus commented Oct 9, 2021

Seems reasonable. Needs a test though

The awscli allows either a section [default] or [profile default] in
the aws config file.
@c42f c42f force-pushed the cjf/config-allow-profile-default branch from 2b30d5b to 21dc437 Compare October 9, 2021 04:52
@c42f
Copy link
Contributor Author

c42f commented Oct 9, 2021

Test added.

Copy link
Member

@mattBrzezinski mattBrzezinski left a comment

Choose a reason for hiding this comment

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

Changes LGTM!

bors try

bors bot added a commit that referenced this pull request Oct 12, 2021
@bors
Copy link
Contributor

bors bot commented Oct 12, 2021

Comment on lines +23 to +35
@testset "default profile section names" begin
allowed_default_sections = ["default", "profile default"]
mktemp() do config_path, _
for default_section_str in allowed_default_sections
config = """
[$default_section_str]
region = xx-yy-1
"""
write(config_path, config)
@test aws_get_region(; profile="default", config=config_path) == "xx-yy-1"
end
end
end
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is the proper syntax.

Suggested change
@testset "default profile section names" begin
allowed_default_sections = ["default", "profile default"]
mktemp() do config_path, _
for default_section_str in allowed_default_sections
config = """
[$default_section_str]
region = xx-yy-1
"""
write(config_path, config)
@test aws_get_region(; profile="default", config=config_path) == "xx-yy-1"
end
end
end
@testset "$(profile) section names" for profile in ["default", "profile_default"]
mktemp() do config_path, _
config = """
[$(profile)]
region = xx-yy-1
"""
write(config_path, config)
@test aws_get_region(; profile="default", config=config_path) == "xx-yy-1"
end
end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, if you want two test sets with a single test in them each, feel free to commit something like this.

Note, however that this patch is wrong because we want to test profile == "profile default" (not "profile_default" with an underscore).

Copy link
Member

Choose a reason for hiding this comment

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

It's nbd, I just find different cases in a @testset are easier to debug.

@mattBrzezinski
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 15, 2021

@bors bors bot merged commit 6934462 into JuliaCloud:master Oct 15, 2021
@c42f c42f deleted the cjf/config-allow-profile-default branch October 27, 2021 03:35
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.

None yet

3 participants