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: use proper bucket for ios videos #1921

Open
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

philippevezina
Copy link
Contributor

@philippevezina philippevezina commented Feb 29, 2024

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 configured recordingsBucket which is where replay.mp4 is uploaded.

If I'm not mistaken, here is where the bucket for the video storage is defined:

value: {{ .Values.global.s3.recordingsBucket }}

Summary by CodeRabbit

  • New Features
    • Updated the configuration for iOS video access to enhance the video retrieval process.
  • Chores
    • Removed obsolete IOS_VIDEO_BUCKET configuration across various environments to streamline configuration settings.
    • Updated configurations for single sign-on URL and caching mechanisms to improve user experience and application performance.

Amirouche and others added 30 commits January 26, 2024 18:11
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
Copy link

coderabbitai bot commented Feb 29, 2024

Walkthrough

The recent updates focus on streamlining video bucket configurations and enhancing security and usability features across different environments. Specifically, the IOS_VIDEO_BUCKET configuration has been deprecated in favor of a more general sessions_bucket naming, aligning with a broader scope of content management. Additionally, adjustments to SSO URLs and invitation link settings further refine the application's integration capabilities and user engagement strategies.

Changes

File Path Change Summary
api/chalicelib/core/sessions_mobs.py Updated bucket name configuration from "IOS_VIDEO_BUCKET" to "sessions_bucket".
api/env.default
api/env.dev
ee/api/env.default
ee/api/env.dev
Removed IOS_VIDEO_BUCKET configuration.
Updated idp_sso_url and invitation_link settings.
Adjusted js_cache_bucket configuration.

🐇✨
In the realm of code, where changes abound,
A rabbit hopped in, making hardly a sound.
"Farewell, old bucket," it said with a leap,
"Hello, new settings, our secrets to keep."
With each line of code, carefully spun,
Our digital warren, brighter than the sun. 🌞
🎉🐰

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

Copy link

@coderabbitai coderabbitai bot left a 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

Commits Files that changed from the base of the PR and between f9aaa45 and 34a9070.
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 the IOS_VIDEO_BUCKET environment variable has been handled correctly.

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

* 40-45: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [63-63]

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 the ee/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.

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

* 41-46: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [63-63]

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 the invitation_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.

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

* 41-46: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [66-66]

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 the sessions_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) to mobs. This consistency supports the update in the get_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

@philippevezina
Copy link
Contributor Author

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.

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.

None yet

8 participants