-
Notifications
You must be signed in to change notification settings - Fork 803
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
Flags used to pass tests become set for all installations #7438
Comments
You are right - I think flags should only be set if they are to be used by end-users: I guess we gave been lucky that this hasn't been a serious problem until now - at least that this was a potential problem had not even occurred to me. I guess in this case we should drop the flag and allow the tests to fail or disable them as needed. I don't really think there is much we can do for the already released snapshots (lts-22.23 and lts-22.24), but we should do that from lts-22.25 at least. |
I note the same is true for nightly too. Also we should document this somewhere: probably in build-constraints.yaml |
Thanks @juhp let me make sure I understand: The plan is to document the fact that the flags in That seems a bit odd -- why wouldn't such a use-case be better-served by a different default in the project's cabal file? |
@pbrisbin, I didn't fully understand your suggestion. But if you are thinking there should be more than one flags sections in Stackage, I don't see how that would work. The thing is Stackage is basically just one huge stack build, so there's no way currently to use such configuration. Also personally I am becoming of the opinion lately that we maybe should consider completely stopping running testsuites in our Stackage builds: they actually take up a considerable amount of the total human time spent on Stackage work... I feel just building the testsuites but not running them might be a saner sweet spot for Stackage. |
Yeah, I wasn't ready to suggest something concrete. It just seemed that "flags to use when Stackage tests this" and "flags to use whenever installed from this resolver" are two very different use cases. And I'd claim the latter is simply not something anyone needs. I admit I don't know the ins-and-outs, it just seemed like somewhere
Complete agreement here. |
It seems in the past, we decided to set a
development:true
flag in one of our packages for lts-22.23.This flag is necessary for the tests to pass, and test failures is what prompted us to do this. It all seemed reasonable at the time, but we had no idea that setting this flag there, to get tests passing on Stackage CI, also meant that everyone installing the package from Stackage ever would build the package with this flag set. Had anyone thought about this, obviously a flag named
development
should probably not be used this way.The result is Very Bad. The flag breaks the security of the package, causing it to validate SNS webhooks as coming from a
localhost
-signed certificate (which we use in tests) rather than an AWS-signed certificate. If we shipped a service with this library built this way, someone could submit a self-signed payload and it would be erroneously accepted as if it were AWS-signed.I'm opening this Issue with two questions:
What is the best thing to do about this now, such that installs from
lts-22.23
no longer get adevelopment:true
-built version ofaws-sns-verify-0.0.0.3
?The situation in general seems like something that should be changed. Do you agree?
It seems totally reasonable, and common, that packages might use flags to change how they work in development/test, that such flags would be needed to pass tests when Stackage CI runs it, but such flags would NOT be needed (and could be harmful) when users install those packages. It thus seems really bad to conflate the flags used to test the packages in Stackage with the flags used when users install from there.
The text was updated successfully, but these errors were encountered: