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

Cloudinary admin url update: fixes #2724 #4175

Merged
merged 1 commit into from
Sep 25, 2017

Conversation

Noviny
Copy link
Contributor

@Noviny Noviny commented Apr 3, 2017

Changes:
Cloudinary fields pass down secure property with their getOptions
call.

When a url is created by the admin UI, the secure property is added. This is handled at each location an image is referenced.

Changes:
Cloudinary fields pass down `secure` property with their `getOptions`
call.

When a url is created, the secure property is added.
@Noviny Noviny changed the title Cloudinary admin url update: fixes 2724 Cloudinary admin url update: fixes #2724 Apr 3, 2017
@sktt
Copy link
Contributor

sktt commented Apr 3, 2017

Does secure imply serve over https? If so why would you ever not want to use https?

@Noviny
Copy link
Contributor Author

Noviny commented Apr 4, 2017

@sktt secure does mean over https, and it was a legacy implementation we are working on moving away from.

@MillerApps
Copy link

Any word on this being merged?

this._originalGetOptions();
// We are performing the check here, so that if cloudinary secure is added
// to keystone after the model is registered, it will still be respected.
// Setting secure overrides default `cloudinary secure`
Copy link
Contributor

Choose a reason for hiding this comment

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

You've got me curious. What's the use case for setting secure/cloudinary secure after the models have been registered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's mostly about the ordering of your keystone.js file. It is possible and valid to import your models and then after that line set cloudinary secure, however rather obtusely without checking again, you can end up not applying the option it looks like you set.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks for the explanation.

Copy link
Contributor

@gwyneplaine gwyneplaine left a comment

Choose a reason for hiding this comment

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

lgtm

@Noviny Noviny merged commit c9f5bc1 into master Sep 25, 2017
@Noviny Noviny deleted the cloudinary-respect-secure-in-admin branch September 25, 2017 23:39
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.

5 participants