-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Orchard.Azure.MediaServices module updates #7618
Orchard.Azure.MediaServices module updates #7618
Conversation
mconverti
commented
Mar 21, 2017
- Upgraded Azure Media Services legacy components (like the encoder and presets)
- Included Azure Media Player v2.0.0.9 for the video playback experience
- Added support to auto-configure the Orchard.Azure.MediaServices module (Media Services and Storage credentials) based on application settings
- Added new 'Video Portal' recipe (based on 'Default' recipe) to enable Orchard.Azure.MediaServices module
- Improved Setup experience to avoid asking for connection string if it's already available in the application settings
…tings + minor bug fixes
…zure-media-services-update-1.10.x
97cb59a
to
2616d2a
Compare
a228c60
to
ebf6683
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have the AMS recipe be available only in the setup if we build the image for Azure?
This would be a new Target in the Orchard.proj
file that would copy the file from the module to the publication folder after build and before packaging.
@@ -1,9 +1,21 @@ | |||
@using Orchard.ContentManagement; | |||
|
|||
@{ | |||
string alternateText = string.Empty; | |||
try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What could go wrong? If a NRE then just check for nullability or us .?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a nullability issue. The original code was failing when embedding a cloud media video item in a page because of a binder issue with the As<> extension method.
This is a quick workaround for the existing error in the code. I think that doing a IContent cast before calling the As<> extension method will fix it; I will push the changes later.
{ | ||
var result = new WebconfigOverrides(); | ||
|
||
if (ConfigurationManager.ConnectionStrings[ConnectionStringDefaultConnectionString] != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it also looking in environment variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, when running in an Azure App Service environment, the configuration settings specified in the Portal (through ARM) are available as environment variables at runtime. For each app setting, two environment variables are created; one with the name specified by the app setting entry, and another with a prefix of APPSETTING_. Both contain the same value.
For .NET apps in particular, these settings are also injected into the .NET configuration AppSettings/ConnectionString at runtime, overriding existing settings. That's why we can transparently use the ConfigurationManager class to read the settings.
|
||
if (ConfigurationManager.ConnectionStrings[ConnectionStringDefaultConnectionString] != null) | ||
{ | ||
result.DataConnectionString = ConfigurationManager.ConnectionStrings[ConnectionStringDefaultConnectionString].ConnectionString; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to prefix with the Tenant's name like tenantName + ":" + ConnectionStringDefaultConnectionString
and fallback to one without the tenant name. Because otherwise this would override the value for subsequent tenants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can use our IPlatformConfigurationAccessor
that will do it for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebastienros how can we get the tenant name? Should we inject any dependency to be able to get this value?
The problem with the tenantName prefix approach is that it will always fallback to the setting without the tenant name, because the current ARM template is only configuring the "defaultConnection" connection string.
Perhaps a better approach would be to skip the default connection string configuration is tenantName != "Default". I think this should be also performed for the Media Services module default configuration as well.
@sebastienros We will wait for your input on this feedback. Thanks!
@sebastienros regarding the AMS recipe, we are not familiar with the Orchard CMS build process and we currently ran out of cycles to apply this improvements. Perhaps you can handle this change with your team. Thanks! |
We can close this (and other Azure Media Services) PR and issue due to #8759. |