Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore(install-script): install.sh script improvements #6857

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

prashant-shahi
Copy link
Member

@prashant-shahi prashant-shahi commented Jan 20, 2025

Summary

  • support for docker compose plugin
  • check for occupied port when installing SigNoz for the first time
  • remove unused code

Important

Improved install.sh by adding Docker Compose plugin support, checking for occupied ports, and removing unused code.

  • Docker Compose Handling:
    • Added has_docker_compose_plugin() to check for Docker Compose plugin.
    • Set docker_compose_cmd to use either docker compose or docker-compose based on availability.
    • Updated all docker-compose commands to use $docker_compose_cmd.
  • Port Check:
    • Added check for occupied ports 3301 and 4317 if SigNoz is not installed.
  • Code Cleanup:
    • Removed commented-out and unused code in install.sh.

This description was created by Ellipsis for d73a0b6. It will automatically update as commits are pushed.

@prashant-shahi prashant-shahi requested a review from a team as a code owner January 20, 2025 13:06
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions github-actions bot added the chore label Jan 20, 2025
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to d73a0b6 in 1 minute and 24 seconds

More details
  • Looked at 172 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. deploy/install.sh:36
  • Draft comment:
    Consider using is_command_present for checking docker compose plugin for consistency.
has_docker_compose_plugin() {
    is_command_present "docker compose"
}
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The function has_docker_compose_plugin checks for the docker compose plugin, but the function is_command_present is used elsewhere to check for command presence. It would be more consistent to use is_command_present for checking docker compose plugin as well.
2. deploy/install.sh:479
  • Draft comment:
    Consider defining port numbers as constants at the top of the script for better maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The check_ports_occupied function uses hardcoded port numbers 3301 and 4317. It would be better to define these as constants at the top of the script for easier maintenance and readability.
3. deploy/install.sh:428
  • Draft comment:
    Duplicate assignment for event variable in send_event function. Remove the redundant line.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_ED6OCRIxi04tLboe


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Signed-off-by: Prashant Shahi <prashant@signoz.io>
@prashant-shahi
Copy link
Member Author

Related Issue

@prashant-shahi prashant-shahi enabled auto-merge (squash) January 20, 2025 13:24
@prashant-shahi prashant-shahi merged commit 610f4d4 into main Jan 20, 2025
16 checks passed
@prashant-shahi prashant-shahi deleted the chore/install-script branch January 20, 2025 13:26
grandwizard28 added a commit that referenced this pull request Jan 20, 2025
feat: add search span scope in the list view tab to add the scope at per Query level (#6810)

* feat: add search span scope in the list view tab to add the scope at per query level

* feat: add search span scope in the list view tab to add the scope at per query level

* feat: add search span scope in the list view tab to add the scope at per query level

* feat: add search span scope in the list view tab to add the scope at per query level

* feat: add search span scope in the list view tab to add the scope at per query level

* feat: add search span scope in the list view tab to add the scope at per query level

* feat: add search span scope in the list view tab to add the scope at per query level

* feat: add search span scope in the list view tab to add the scope at per query level

* feat: add search span scope in the list view tab to add the scope at per query level

* feat: add search span scope in the list view tab to add the scope at per query level

* feat: add search span scope in the list view tab to add the scope at per query level

chore(install-script): install.sh script improvements (#6857)

- support for docker compose plugin
- check for occupied port when installing SigNoz for the first time
- remove unused code

Signed-off-by: Prashant Shahi <prashant@signoz.io>

chore: correct order within page result (#6847)
@RecoX
Copy link

RecoX commented Jan 21, 2025

This is not working

@prashant-shahi
Copy link
Member Author

@RecoX can you please share the commands that you followed and also the OS and arch type?

amlannandy pushed a commit that referenced this pull request Jan 21, 2025
### Summary

- support for docker compose plugin
- check for occupied port when installing SigNoz for the first time
- remove unused code

Signed-off-by: Prashant Shahi <prashant@signoz.io>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants