-
Notifications
You must be signed in to change notification settings - Fork 106
fix(agents): correctly parse and validate min sdk version #658
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
base: main
Are you sure you want to change the base?
Conversation
27b0cd3
to
b3f9860
Compare
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.
Something we didn't discuss is what about git installs? We will see people with critical bugs in production who need to hotfix them ASAP and want to ship from git while the PR is merged and a new release is made. Whether from their own fork or from a branch on the main repo.
What will our checker do?
b3f9860
to
c75dcc7
Compare
c75dcc7
to
0797e2e
Compare
@bcherry that's a great point, and not an easy one to deal with. We would have to clone the repo at the specified hash and then do project detection in the repo, which is a bigger can of worms, since they could use any build tools. Should we quietly allow this? |
There's also the issue that the Dockerfile can literally do anything it wants after we check. It could install different package versions than specified in the lockfile or package file. I think we need a runtime solution. |
I added handling for git (as I had to test it directly from git before). it attributes it as |
That works for the base case, but you can totally specify a commit hash, branch, tag, etc. too. I might revert some of this and turn this into warnings, and we go ahead with implementing runtime check. |
Improve minimum SDK version check by supporting semver operators better and treating lockfile version and package file versions separately.