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

fix: load token from shared config dir #51

Merged
merged 3 commits into from
Feb 11, 2021

Conversation

erezrokah
Copy link
Contributor

@erezrokah erezrokah commented Feb 9, 2021

Please forgive my lack of go knowledge.

This is pending #50 so I can test this on Windows.

Kind of required for netlify/cli#1489.

In netlify/cli#1725 we changed the location of config.json without realizing it is referenced in this client and in https://github.com/netlify/netlify-lm-plugin.

netlify/cli#1489 handles the plugin part, this PR is intended to handle the go client part.

@erezrokah erezrokah added the type: bug code to address defects in shipped code label Feb 9, 2021
if err != nil {
return err
}

args := append([]string{home}, validAuthPaths[0]...)
f, err := os.OpenFile(filepath.Join(args...), os.O_CREATE|os.O_RDWR, 0644)
f, err := os.OpenFile(filepath.Join(validAuthPaths[0]...), os.O_CREATE|os.O_RDWR, 0644)
Copy link
Contributor Author

@erezrokah erezrokah Feb 9, 2021

Choose a reason for hiding this comment

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

Question - should we make sure to create the nested directory path if the file doesn't exist?

Copy link
Contributor

Choose a reason for hiding this comment

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

another #lazyweb but what xdg.ConfigHome looks like usually? (or APPDATA)
I feel like at least the netlify (and Config for windows) directory needs to be created regardless?
how are we doing in CLI side?

Copy link
Contributor Author

@erezrokah erezrokah Feb 10, 2021

Choose a reason for hiding this comment

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

Commented on the structure in #51 (comment)

The CLI uses configstore to access the file so it handles directory creation https://github.com/yeoman/configstore/blob/ab81102b4917fda25d2e2ec616a00ea9e9227a36/index.js#L59

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added another commit for this 1497e36

A follow up question - should we change the permissions for the file/directories to 0600 or 0640 to prevent others reading the file?

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like directories are 700 and file is 600 with CLI (aka configstore)?

https://github.com/yeoman/configstore/blob/ab81102b4917fda25d2e2ec616a00ea9e9227a36/index.js#L13-L14

I'd vote for being as consistent as possible with CLI here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 2978889

@erezrokah erezrokah force-pushed the fix/add_new_config_json_locations branch 4 times, most recently from 9073e99 to 6ad3bb2 Compare February 9, 2021 17:34
@erezrokah erezrokah force-pushed the fix/add_new_config_json_locations branch from 6ad3bb2 to 9b1c841 Compare February 9, 2021 17:54
@keiko713
Copy link
Contributor

thanks for the PR!! overall looking good, left some questions so that I understand better

Copy link
Contributor

@keiko713 keiko713 left a comment

Choose a reason for hiding this comment

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

looks very good!!! thanks for this update 🎉 did you test locally for this? I happen to have a new laptop that hasn't installed anything, so I'm happy to try it out the whole flow if you want

@erezrokah
Copy link
Contributor Author

did you test locally for this

I built the helper locally and was able to upload files with the dev version:
https://github.com/erezrokah/netlify-cms-large-media/blob/main/static/media/netlify_2.png

For some reason I can't see them under the large media tab in the UI though. I'll look into it

@erezrokah
Copy link
Contributor Author

erezrokah commented Feb 11, 2021

For some reason I can't see them under the large media tab in the UI though. I'll look into it

OK, so the files show up when I use the CMS.

Our UI uses https://api.netlify.com/api/v1/large_media_assets/?site_id=<site_id>.
The CMS uses https://netlify-cms-large-media.netlify.app/.netlify/large-media/origin/<sha> to get the file directly (it gets the sha from the repo).
Our open API specs for large media points to the assets api.

I'm confused, but it seems the credential helper works.

@keiko713
Copy link
Contributor

Ok hope you don't mind my terrible sketch:

image

Likely the reason that you're not seeing the images in "Netlify UI LM page" (left bottom in diagram) is that you don't have any reference to the pointer file with built site (step 6 didn't happen).
However, from Netlify CMS point of view, since it directly refers the images from LM storage (that's https://netlify-cms-large-media.netlify.app/.netlify/large-media/origin/<sha> is about), you can see the image.

Our open API specs for large media points to the assets api.

This is a documentation bug, this assets endpoint is sadly completely unrelated to the large media.

Hope this explains!

@erezrokah
Copy link
Contributor Author

Likely the reason that you're not seeing the images in "Netlify UI LM page" (left bottom in diagram) is that you don't have any reference to the pointer file with built site (step 6 didn't happen).

This is awesome! Thank you so much for explaining it to me 🚀

@erezrokah erezrokah merged commit 961f8a9 into master Feb 11, 2021
@erezrokah erezrokah deleted the fix/add_new_config_json_locations branch February 11, 2021 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants