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

Check for latest commit before install.sh #1186

Merged
merged 9 commits into from
Dec 6, 2021

Conversation

aminvakil
Copy link
Collaborator

Fixes #925

However this would prevent from running in isolated (without Internet) environments.

Credit: https://stackoverflow.com/a/25109122/3835210

@aminvakil
Copy link
Collaborator Author

Current state has problems when running on tags...

@aminvakil aminvakil force-pushed the check_latest_version branch from 991d2c9 to a569bad Compare December 4, 2021 12:51
@aminvakil aminvakil force-pushed the check_latest_version branch from a569bad to b4db562 Compare December 4, 2021 12:53
Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Quite sure this would break all the releases. This should be limited to nightly builds only and since we don't promote using those anymore, I'd say fixing #925 is more hassle than it is worth.

@BYK
Copy link
Member

BYK commented Dec 4, 2021

This also assumes the repo is run from a git checkout, which is not the case (for the e2e tests Sentry runs using this repo for instance)

@aminvakil
Copy link
Collaborator Author

Quite sure this would break all the releases. This should be limited to nightly builds only and since we don't promote using those anymore, I'd say fixing #925 is more hassle than it is worth.

This checks if master branch has been checked out, otherwise skip, I can't understand what you mean by breaking all the releases.

This also assumes the repo is run from a git checkout, which is not the case (for the e2e tests Sentry runs using this repo for instance)

Thanks! Fixed by bcf256e.

@BYK
Copy link
Member

BYK commented Dec 6, 2021

This checks if master branch has been checked out, otherwise skip, I can't understand what you mean by breaking all the releases.

Ah, sorry I missed that logic. Doing another review now.

Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Requesting change for comments and a bypass logic, otherwise seems good.

There's also a failure in integration tests but don't know if this is relevant or not.

install.sh Outdated
Comment on lines 9 to 10
if [[ $(git branch | sed -n '/\* /s///p') == "master" ]]; then
if [[ $(git rev-parse HEAD) != $(git ls-remote $(git rev-parse --abbrev-ref @{u} | sed 's/\// /g') | cut -f1) ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Some comments here explaining the logic would be very beneficial.

I also think we should have a flag (CLI or env variable) to bypass this logic as I can easily see this becoming a nuisance for people maintaining their custom versions.

@aminvakil
Copy link
Collaborator Author

Requesting change for comments and a bypass logic, otherwise seems good.

👍

There's also a failure in integration tests but don't know if this is relevant or not.

I think it's related to #1171

@aminvakil
Copy link
Collaborator Author

I don't like --not-latest-commit switch, any suggestions?

Copy link
Member

@chadwhitacre chadwhitacre left a comment

Choose a reason for hiding this comment

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

Can we fold into the existing structure?

  • command line switch goes in parse-cli.sh
  • new file called install/check-latest-commit.sh or somesuch

Wdyt?

@chadwhitacre
Copy link
Member

I don't like --not-latest-commit switch, any suggestions?

Maybe --skip-commit-check? 🤔

@aminvakil
Copy link
Collaborator Author

Can we fold into the existing structure?

* command line switch goes in `parse-cli.sh`

* new file called `install/check-latest-commit.sh` or somesuch

Wdyt?

Nice idea.

@chadwhitacre chadwhitacre merged commit 3c060fc into getsentry:master Dec 6, 2021
@chadwhitacre chadwhitacre added this to the Prevent Component Drift milestone Dec 6, 2021
@aminvakil aminvakil deleted the check_latest_version branch December 7, 2021 08:30
@chadwhitacre chadwhitacre mentioned this pull request Dec 15, 2021
3 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent an old version of onpremise being run with latest Sentry/Snuba/Relay
3 participants