Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Ensure credential paths are "Finalized" to their contents. #8048
Conversation
|
Can one of the admins verify this patch? |
ExternalReality
changed the title from
Ensure credential filepaths are to their contents.
to
WIP: Ensure credential filepaths are to their contents.
Nov 10, 2017
ExternalReality
changed the title from
WIP: Ensure credential filepaths are to their contents.
to
WIP: Ensure credential paths are "Finalized" to their contents.
Nov 10, 2017
ExternalReality
changed the title from
WIP: Ensure credential paths are "Finalized" to their contents.
to
Ensure credential paths are "Finalized" to their contents.
Nov 13, 2017
| } | ||
| return credential, credentialName, regionName, nil | ||
| } | ||
| +// FinalizeFileContent replaces the path content of cloud credentials "file" attribute with its content. | ||
| +func FinalizeFileContent(credential *cloud.Credential, provider environs.EnvironProvider) (*cloud.Credential, error) { |
ExternalReality
Nov 13, 2017
•
Edited 10 times
-
ExternalReality
Nov 15, 2017
-
ExternalReality
Nov 15, 2017
-
ExternalReality
Nov 13, 2017
-
ExternalReality
Nov 13, 2017
-
ExternalReality
Nov 13, 2017
-
ExternalReality
Nov 13, 2017
-
ExternalReality
Nov 13, 2017
-
ExternalReality
Nov 13, 2017
-
ExternalReality
Nov 13, 2017
-
ExternalReality
Nov 13, 2017
Member
-
Nov 15, 2017 -
Nov 15, 2017 -
Nov 13, 2017 -
Nov 13, 2017 -
Nov 13, 2017 -
Nov 13, 2017 -
Nov 13, 2017 -
Nov 13, 2017 -
Nov 13, 2017 -
Nov 13, 2017
This function has been factored out of the existing code. Part of the reason for this bug is there is limited testing around this particular area of code. No test verifies that the variable storing the file path is actually converted to store its contents. Three things come to my mind here; we can either inject the readFile function into all relevant functions so that we can "mock" the file reading for test. Alternatively, we could use a file I/O mocking library. Lastly, we can make an actual temporary file for testing. I am aware of the fundamental Pros and Cons of each method. Is there an area of the code, that favors one approach over the other, which I can mimic here?
Another thing to do might be to relocate FinalizeFileContent so that it would live in Cloud, where it might be added to the interface, and stub the method altogether for testing. I welcome any feedback regarding this issue.
|
!!build!! |
jameinel
approved these changes
Nov 15, 2017
I just realized I didn't comment on this.
I think its better than what we had, I was trying to find ways to help you work out tests for everything. We can try to talk it over tomorrow. Lets talk about it during standup.
|
!!build!! |
|
$$MERGE$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
ExternalReality commentedNov 10, 2017
•
Edited 2 times
-
ExternalReality
Nov 14, 2017
-
ExternalReality
Nov 10, 2017
Description of change
When bootstrapping a controller on
GCEwithJUJU_DATAset to the path of an empty directory, Juju will try to look for a path to a file holdingGCEcredentials in the environment variableGOOGLE_APPLICATION_CREDENTIALS. Juju incorrectly tries to use the file path itself as the credentials instead of using the contents of that file. This PR fixes that error.Why is this change needed?
Fixes referenced bug.
QA steps
GOOGLE_APPLICATION_CREDENTIALSto the path of your application credentials.JUJU_DATAto an empty directory (This wayJujuignores its current set of credentials.)GCEwhereas without this patch it would error expecting the file path string to be validJSON.Documentation changes
N/A
Does it affect current user workflow? CLI? API?
No
Bug reference
https://bugs.launchpad.net/juju/+bug/1726226