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

feat: add prometheus build config #198

Merged
merged 6 commits into from
Jan 11, 2024
Merged

feat: add prometheus build config #198

merged 6 commits into from
Jan 11, 2024

Conversation

jayl1e
Copy link
Contributor

@jayl1e jayl1e commented Jan 10, 2024

Why:

  • add prometheus config

@ti-chi-bot ti-chi-bot bot added the size/L label Jan 10, 2024
Copy link

ti-chi-bot bot commented Jan 10, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary of Changes

The changes presented in this pull request mainly involve adding Prometheus to the build configuration. Here are some key changes:

  1. The version constraint for release was updated from >= 6.1.0-0 to >= 7.5.0-0.
  2. A new artifact named container image was added, which includes the Dockerfile from the referenced GitHub URL and the ng-monitoring-server from the local bin directory.
  3. The artifact ng-monitoring-{{ .Release.version }}-{{ .Release.os }}-{{ .Release.arch }}.tar.gz was renamed to prometheus-{{ .Release.version }}-{{ .Release.os }}-{{ .Release.arch }}.tar.gz.
  4. The Prometheus binary and several Prometheus rule files were added as new files to be included in the tarball.
  5. The same set of changes was repeated for 3 different version ranges >=v5.3.0-0 <7.5.0-0, >=v5.2.0-0 <5.3.0-0.

Potential Problems

  1. Hardcoded version: The version of Prometheus being downloaded (2.8.1) is hardcoded. This could become an issue if you want to support different versions of Prometheus.
  2. Missing PR description: The PR description is missing which should have included the reason for these changes and any potential impact.
  3. Repeated Code: There is a lot of repeated code for the different version ranges. This could be a maintenance problem if changes need to be made in the future.
  4. External Dependencies: The build process now depends on several external URLs. If any of these URLs become unavailable, the build will fail.
  5. Lack of Error Handling: There's no error handling in case the download or extraction of Prometheus fails.

Suggestions

  1. Parameterize the version: Consider making the Prometheus version a variable that can be injected at build time or pulled from a configuration file.
  2. Provide a PR description: Always include a detailed description with your PRs to give context to reviewers.
  3. DRY (Don't Repeat Yourself): Try to refactor the code to avoid repetition. If the build steps for the different versions are the same, consider using a loop or a function.
  4. Add Error Handling: Add checks in your build script to handle failures in downloading or extracting Prometheus.
  5. Consider using a sub-module or package manager: Instead of downloading Prometheus during the build, you could use a Git sub-module or a package manager to handle the Prometheus dependency. This way, you can pin to a specific version and reduce the risk of the build failing due to an unavailable URL.

@jayl1e
Copy link
Contributor Author

jayl1e commented Jan 10, 2024

/hold

Copy link

ti-chi-bot bot commented Jan 10, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Key Changes

  1. The pull request adds Prometheus build configurations to the project. Prometheus is a monitoring system and time-series database.
  2. The README file is updated to include references for SemVer Constraint and changes in the links for yq and jq.
  3. The GoLang version used by the builders is now documented in the README file.
  4. The packages.yaml.tmpl file is modified to include the configuration for Prometheus and its related components.
  5. The build scripts build-package-artifacts.sh.tmpl and build-package-images.sh.tmpl are updated to handle the extraction of files correctly.

Potential Problems

  1. Hard-coded version numbers: The version of Prometheus (2.8.1) is hard-coded in the configuration. This might make it difficult to update the version in the future.
  2. Dependencies on external resources: The build configuration is downloading a number of rules from external URLs. If these URLs become unavailable in the future, the build will fail.
  3. Unverified downloads: The build configuration is downloading files from external sources but doesn't seem to be verifying them. This might be a potential security risk.

Suggestions

  1. Use version variables: Instead of hard-coding version numbers, it might be better to use variables. This would make it easier to update the version number in the future.
  2. Store external dependencies in the project: If possible, external dependencies should be included in the project itself. This would make the build process more reliable.
  3. Verify downloads: The build configuration should verify the downloads from external sources to ensure their integrity and authenticity. This could be done using checksums or digital signatures.

Here's how you could format your response in markdown:

# Review of PR: feat: add Prometheus build config

## Key Changes
1. Addition of Prometheus build configurations.
2. Updates to README file including references and links.
3. Documentation of GoLang version used by builders added to README.
4. Updates to `packages.yaml.tmpl` file to include Prometheus configuration.
5. Updates to `build-package-artifacts.sh.tmpl` & `build-package-images.sh.tmpl` to handle file extraction.

## Potential Issues
1. Hard-coded version numbers: Could cause difficulty in future updates.
2. Dependencies on external resources: Could lead to build failures if URLs become unavailable. 
3. Unverified downloads: Potential security risk.

## Suggestions for Improvement
1. Use version variables instead of hard-coded versions.
2. Store external dependencies in the project.
3. Verify downloads for improved security.

Copy link

ti-chi-bot bot commented Jan 10, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Review

This pull request adds Prometheus build configuration to the project. It makes significant changes to the packages.yaml.tmpl file, where it adds the Prometheus binary and its configuration files to the build process.

Prometheus is a popular open-source monitoring system and time series database. It provides a multi-dimensional data model and a flexible query language and is extensively used in cloud computing for infrastructure and application monitoring.

Here are the key changes:

  1. The .gitignore file has been updated to ignore some shell script files.

  2. Changes in the README.md file include addition of a reference link and updates to the golang versions used by the builders.

  3. The packages.yaml.tmpl file has been significantly updated to include the build configuration for Prometheus and its associated components.

  4. The build-package-artifacts.sh.tmpl and build-package-images.sh.tmpl files have been updated to include the extraction of the Prometheus binary from its tar.gz file.

Potential Issues:

  1. Direct downloads from URLs: The download URLs for the Prometheus binary and other configuration files are hardcoded in the packages.yaml.tmpl file. This may lead to problems if these URLs change or become unavailable in the future.

  2. Versioning of Prometheus: The Prometheus version (2.8.1) is hardcoded in the file. This might cause issues when there's a need to use a different version of Prometheus.

  3. Versioning of Golang: There are some changes in the golang versions used by the builders. This needs to be verified if it's compatible with the rest of the components.

Suggestions:

  1. Use environment variables or a configuration file for URLs and versions to make the script more flexible and maintainable.

  2. Consider using a package manager or a more automated method to handle versions of Prometheus and Golang.

  3. Test the new Golang version with all components to ensure compatibility.

Here is a markdown formatted summary:

## Summary

This pull request introduces the Prometheus build configuration to the project. Key updates are made to the packages.yaml.tmpl file, where the Prometheus binary and its configuration files are added to the build process.

## Key Changes

1. Update to .gitignore file to ignore certain shell scripts.
2. Modification of README.md file to include a reference link and specify the Golang versions used by builders.
3. Significant changes to packages.yaml.tmpl file to include build configuration for Prometheus and its components.
4. Update to build-package-artifacts.sh.tmpl and build-package-images.sh.tmpl to handle the extraction of the Prometheus binary.

## Potential Issues

1. Direct downloads from URLs: The URLs for the Prometheus binary and other files are hardcoded.
2. Versioning: The Prometheus version is hardcoded in the script.
3. Golang version: Changes in the Golang version used by the builders may affect compatibility.

## Suggestions

1. Use environment variables or a configuration file for URLs and versions for maintainability.
2. Consider using a package manager or an automated method for handling the versions of Prometheus and Golang.
3. Test the new Golang version with all components for compatibility.

jayl1e and others added 3 commits January 10, 2024 19:14
Signed-off-by: lijie <lijie@pingcap.com>
Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Copy link

ti-chi-bot bot commented Jan 10, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Review of Pull Request

Summary of Changes

  1. The .gitignore file is updated to ignore some scripts.
  2. The README file is updated to add more documentation regarding the Golang versions the builders are using. Additionally, it provides corrected links for the tools yq and jq.
  3. The packages.yaml.tmpl file is updated, removing some redundant version comments, and adding a significant number of changes to the components, particularly the Prometheus component. The changes include adding a lot of .src fields with URLs to fetch different Prometheus configurations.
  4. The build-package-artifacts.sh.tmpl and build-package-images.sh.tmpl scripts are updated to adjust the logic of the archive function to handle src fields with extract and extract_inner_path.

Potential Problems

  1. The packages.yaml.tmpl file has a massive addition of configurations fetched from different repositories. If any of these repositories change the location of these configurations or the repositories are not available, it will cause the build to fail. This represents a potential maintainability issue.
  2. The scripts build-package-artifacts.sh.tmpl and build-package-images.sh.tmpl have been edited to handle extract_inner_path. This could potentially introduce bugs if not tested properly.
  3. HTTP requests without any sort of authentication or rate limit handling could potentially lead to problems if the external server starts to refuse requests due to too many requests.

Suggestions

  1. Consider having a fallback or mirror for the configurations fetched from different repositories, or consider storing these configurations within the project repository if they are not changed frequently.
  2. Thoroughly test the updated scripts build-package-artifacts.sh.tmpl and build-package-images.sh.tmpl to ensure that they behave as expected.
  3. Implement a way to handle failed HTTP requests gracefully, such as retries with exponential backoff, or consider using authenticated requests if possible.

Signed-off-by: lijie <lijie@pingcap.com>
Copy link

ti-chi-bot bot commented Jan 10, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Pull Request Review Summary

Key Changes

  • The addition of the Prometheus build config to the project.
  • Changes in the README file to add more information about the SemVer Constraint and correct some typos.
  • In the packages.yaml.tmpl file, there are significant modifications to the components and their respective configurations.
  • The .gitignore file has been updated to include certain scripts in the packages/scripts directory.
  • The scripts build-package-artifacts.sh.tmpl and build-package-images.sh.tmpl have been updated to handle the extraction of files from different sources like local files, ORAS, and HTTP.
  • There are also modifications in the ci.sh file to include the ng-monitoring component in the CI process.

Potential Problems

  • The extraction logic in build-package-artifacts.sh.tmpl and build-package-images.sh.tmpl may not work correctly if the extract_inner_path is not provided or if it doesn't match the actual structure of the tarball.
  • The Git refs used in extracting the rules YAML files from HTTP sources are not verified or validated. If these refs are invalid or inaccessible, it may cause the script to fail.
  • The scripts being ignored in the .gitignore file are important for building package artifacts and images. Ignoring changes to these files might cause issues if updates are made in the future.
  • The current packages.yaml.tmpl configuration only supports a single golang version per range of versions of the component. If a component needs to be built with different golang versions within the same range, this configuration will not support it.

Suggestions

  • Add error handling or validation to make sure extract_inner_path is provided and correctly points to the desired location inside the tarball.
  • Validate the Git refs used in the HTTP sources to ensure they are accessible and point to valid files.
  • Consider not ignoring the scripts in the .gitignore file as they are important for building package artifacts and images.
  • If needed, modify the packages.yaml.tmpl configuration to support multiple golang versions for the same version range of a component. This may involve restructuring the current configuration format.

Copy link

ti-chi-bot bot commented Jan 10, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Review Summary

This pull request mainly focuses on adding a build config for Prometheus and updates the scripts for generating package artifacts and images. The changes are mostly additions and updates to current scripts and templates.

Key Changes

  1. In packages.yaml.tmpl, the build config for Prometheus has been added. This includes the addition of source URLs for the Prometheus binary and associated rule files.

  2. In build-package-artifacts.sh.tmpl and build-package-images.sh.tmpl, changes have been made to handle the extraction of the downloaded Prometheus binary.

  3. In .gitignore, build-package-artifacts.sh, build-package-images.sh, and compose-offline-packages-artifacts.sh have been added.

  4. In README.md, the Go version used by the builders is now specified. Furthermore, links for yq and jq tools have been added to the Prerequire tools section.

  5. In ci.sh, ng-monitoring has been added to the list of components.

Potential Problems

  1. Hardcoded Versions: The version of Prometheus being fetched is hardcoded (prometheus-2.8.1). This could lead to maintenance issues in the future, as updates to Prometheus won't be automatically reflected.

  2. Direct Downloads: The scripts directly download files from GitHub and other sources. This could be a problem if the structure of the repositories changes or if the files are moved or deleted.

Suggestions

  1. Versioning: Consider using a configuration file or environment variable to manage the version of Prometheus. This would make it easier to update the version in the future.

  2. Error Handling: The scripts do not appear to handle errors that may occur during the download or extraction process. Consider adding error handling to these scripts to make them more robust.

  3. Dependency Management: Instead of directly downloading from external sources, consider using a package manager or a more controlled means of fetching dependencies. This could help prevent issues if the external source changes or goes down.

Markdown Format Response

# Pull Request Review

## Summary

This pull request mainly adds a build configuration for Prometheus and updates the scripts for generating package artifacts and images. 

## Key Changes

1. Addition of build configuration for Prometheus in `packages.yaml.tmpl`.
2. Updates in `build-package-artifacts.sh.tmpl` and `build-package-images.sh.tmpl` to handle the extraction of the downloaded Prometheus binary.
3. Addition of `build-package-artifacts.sh`, `build-package-images.sh`, and `compose-offline-packages-artifacts.sh` in `.gitignore`.
4. Updates in `README.md` to specify the Go version used by the builders and addition of `yq` and `jq` tools links.
5. Addition of `ng-monitoring` in the list of components in `ci.sh`.

## Potential Issues

1. The version of Prometheus being fetched is hardcoded (`prometheus-2.8.1`) which could lead to maintenance issues in the future.
2. The scripts directly download files from GitHub and other sources which could be a problem if the structure of the repositories changes or if the files are moved or deleted.

## Suggestions

1. Consider using a configuration file or environment variable to manage the version of Prometheus.
2. Consider adding error handling to the scripts to make them more robust.
3. Instead of directly downloading from external sources, consider using a package manager or a more controlled means of fetching dependencies. 

Signed-off-by: lijie <lijie@pingcap.com>
Copy link

ti-chi-bot bot commented Jan 11, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Pull Request Review

Summary of changes

  • The PR introduces Prometheus build configuration.
  • It makes changes to .gitignore, README.md, packages.yaml.tmpl, build-package-artifacts.sh.tmpl, build-package-images.sh.tmpl, and ci.sh.
  • It adds scripts for building package artifacts and images.
  • It updates the README file to include more information about the version of Golang being used, and adds related links for tools like yq and jq.
  • It also moves the SemVer constraints reference link to a more relevant section.
  • It modifies the packages.yaml.tmpl to include more components and their specific configurations.
  • The changes in the scripts seem to be about improving the archive and extraction process for components.

Potential Issues

  1. Hardcoding versions: The PR hardcodes the versions of some components/tools (e.g., https://download.pingcap.org/prometheus-2.8.1.linux-amd64.tar.gz). This might make it hard to maintain in the future if the version updates. It would be better to use variables for these versions.

  2. Lack of testing: The PR doesn't seem to include any new tests for the added scripts and changes. It's important to ensure that the new functionality is working as expected and doesn't break anything.

  3. The changes in packages.yaml.tmpl are quite large and complex. It would be better to have a more detailed explanation about these changes in the PR description.

Suggestions

  1. Use variables for versions and other hardcoded values where applicable.
  2. Ensure to add tests for the new features or modifications.
  3. Update the PR description to include a more detailed explanation about the changes in packages.yaml.tmpl.

Please note that these are initial observations based on the provided PR content, and a complete review might require more context and running the codebase.

@jayl1e
Copy link
Contributor Author

jayl1e commented Jan 11, 2024

/unhold

Copy link
Contributor

@wuhuizuo wuhuizuo left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Copy link

ti-chi-bot bot commented Jan 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the lgtm label Jan 11, 2024
Copy link

ti-chi-bot bot commented Jan 11, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-01-11 06:27:56.8057938 +0000 UTC m=+511066.390047481: ☑️ agreed by wuhuizuo.

@ti-chi-bot ti-chi-bot bot added the approved label Jan 11, 2024
@ti-chi-bot ti-chi-bot bot merged commit 924768d into main Jan 11, 2024
2 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feat/prometheus branch January 11, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants