-
Notifications
You must be signed in to change notification settings - Fork 480
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: use proper bucket for ios videos #1921
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
Co-authored-by: keshav-multi <keshav@multivariate.tech>
Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
* Updated hero * Fixed typo and added feature flags * Typo
* fix(scrips) fix docker installation commands * fix(scripts) check docker compose installation * fix(script) revert scripts styles
Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
Signed-off-by: Rajesh Rajendran <rjshrjndrn@gmail.com>
Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
* fix(tracker): change canvas scaling * fix(tracker): 12.0.3 * fix(tracker): 12.0.3
Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
* change(ui): iframe check for sso redirect * change(ui): removed login.js * change(ui): sso link * change(ui): sso link * change(ui): iframe check and sso redirect
Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
For new minio, the command changed for setting access. Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
Signed-off-by: rjshrjndrn <rjshrjndrn@gmail.com>
WalkthroughThe recent updates focus on streamlining video bucket configurations and enhancing security and usability features across different environments. Specifically, the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- api/chalicelib/core/sessions_mobs.py (1 hunks)
- api/env.default (1 hunks)
- api/env.dev (1 hunks)
- ee/api/env.default (1 hunks)
- ee/api/env.dev (1 hunks)
Files skipped from review due to trivial changes (2)
- api/env.default
- api/env.dev
Additional comments: 6
ee/api/env.dev (2)
- 40-45: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [60-60]
The removal of the
IOS_VIDEO_BUCKET
environment variable aligns with the PR objectives to ensure that the iOS video storage bucket is dynamically determined rather than hardcoded. This change is crucial for addressing the issue with viewing iOS recordings stored on S3. Ensure that all references to this variable are removed or updated across the codebase to prevent any potential issues.Verification successful
The search for any remaining references to
IOS_VIDEO_BUCKET
in the codebase did not yield any results, indicating that all such references have been successfully removed or updated. This aligns with the PR objectives and suggests that the removal of theIOS_VIDEO_BUCKET
environment variable has been handled correctly.* 40-45: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [63-63]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to IOS_VIDEO_BUCKET in the codebase. rg "IOS_VIDEO_BUCKET"Length of output: 21
The update to the
idp_sso_url
declaration, setting it to an empty value, might impact Single Sign-On (SSO) functionality if this environment file is used in a production or staging environment. It's important to verify that this change is intentional and that appropriate configurations are applied elsewhere to ensure SSO continues to work as expected.ee/api/env.default (3)
- 41-46: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [60-60]
The removal of the
IOS_VIDEO_BUCKET
configuration from the default environment file is consistent with the PR's goal to dynamically determine the iOS video storage bucket. This change is a positive step towards resolving the issue with iOS recordings. Similar to the previous file, ensure that there are no lingering references to this variable in the codebase.Verification successful
The search for any remaining references to
IOS_VIDEO_BUCKET
in the codebase did not produce any output, indicating that the removal of this configuration from theee/api/env.default
file was thorough and there are no lingering references. This aligns with the PR's goal to dynamically determine the iOS video storage bucket and suggests that the change was implemented correctly.* 41-46: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [63-63]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to IOS_VIDEO_BUCKET in the codebase. rg "IOS_VIDEO_BUCKET"Length of output: 21
Updating the
invitation_link
configuration to include a token placeholder is a good practice for dynamic URL generation. This change likely enhances the flexibility of user invitation links. Ensure that the token replacement logic is correctly implemented wherever this configuration is used.Verification successful
The verification process revealed that the
invitation_link
configuration does indeed incorporate token replacement logic, using a percentage (%
) format for inserting theinvitation_token
into the URL. This implementation is found in both./api/chalicelib/core/users.py
and./ee/api/chalicelib/core/users.py
, confirming that the token replacement logic is correctly implemented, albeit differently than initially anticipated in the review comment. Therefore, the original review comment's concern has been addressed.* 41-46: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [66-66]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct implementation of token replacement logic for invitation_link. ast-grep --lang python --pattern $'config("invitation_link").format(token=$_)'Length of output: 79
Script:
#!/bin/bash # Broaden the search to find any usage of "invitation_link" in the Python codebase. ast-grep --lang python --pattern $'config("invitation_link")'Length of output: 289
The update to the
js_cache_bucket
configuration setting is noted. It's important to ensure that this change aligns with the intended use of the JavaScript cache bucket across the application. Verify that the new bucket name is correctly configured in the S3 service and that access permissions are properly set.Verification successful
The verification process has confirmed that the
js_cache_bucket
configuration setting is updated across various parts of the application, including environment configuration files, Python code, and Helm chart templates. This suggests that the update to the bucket name is consistently applied. However, ensuring that the new bucket name is correctly configured in the S3 service and that access permissions are properly set is a concern that requires manual verification or confirmation from the application's deployment and security configuration team. The review comment is accurate in highlighting the importance of verifying these aspects.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of js_cache_bucket across the application. rg "js_cache_bucket"Length of output: 1021
api/chalicelib/core/sessions_mobs.py (1)
- 60-63: The update in the
get_ios_videos
function to use the"sessions_bucket"
configuration for checking existence and generating presigned URLs is a direct response to the PR objectives. This change ensures that the correct, user-configured S3 bucket is used for iOS video storage, addressing the issue with viewing iOS recordings. It's important to verify that thesessions_bucket
configuration is correctly set across all environment files to ensure consistency.Verification successful
The verification process confirms that the
sessions_bucket
configuration is consistently set across all environment files (env.default
,env.dev
for both standard and enterprise editions) tomobs
. This consistency supports the update in theget_ios_videos
function to use the"sessions_bucket"
configuration for checking existence and generating presigned URLs, ensuring that the correct, user-configured S3 bucket is used for iOS video storage. The review comment is therefore verified as accurate and consistent with the codebase and its configurations.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct setting of sessions_bucket across all environment configurations. rg "sessions_bucket" ee/api/env.* api/env.*Length of output: 228
Would it be possible for someone to take a look at this? It is currently not possible to play recordings stored on S3 because of this. |
Using S3 to store my recordings, it is currently impossible to view an iOS recording because the prresigned URL uses the
mobs
bucket instead of my configuredrecordingsBucket
which is wherereplay.mp4
is uploaded.If I'm not mistaken, here is where the bucket for the video storage is defined:
openreplay/scripts/helmcharts/openreplay/charts/videostorage/templates/deployment.yaml
Line 69 in f9aaa45
Summary by CodeRabbit
IOS_VIDEO_BUCKET
configuration across various environments to streamline configuration settings.