-
Notifications
You must be signed in to change notification settings - Fork 58
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
CPLAT-13170 Analyze over_react examples without generated code #676
CPLAT-13170 Analyze over_react examples without generated code #676
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
8b7d195
to
22ced1c
Compare
a3e59fb
to
2509241
Compare
It might be easier to review changes by commit (see changes in PR description) |
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.
This looks great to me! I also don't know builders super well though, so the config options are a little beyond me haha
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.
Changes look great! Just one small comment.
I also noticed that the dev build is actually failing and not getting to the post-analysis step :/, which would be really nice to have running.
It looks like the build_vm_compilers|entrypoint
builder might be the culprit. It only serves as an optimization when running VM tests, and isn't required; could you try removing that?
It should involve just removing (or maybe commenting out since we will probably want to turn it back on at some point) the build_vm_compilers|entrypoint
section of build.yaml, and also removing the build_vm_compilers
dep from pubspec.yaml.
.github/workflows/dart_ci.yml
Outdated
- name: Analyze example source (pre-build) | ||
run: | | ||
cd example/boilerplate_versions | ||
if [ ${{ matrix.sdk }} = 'stable@2.7.2' ]; then pub global activate tuneup && tuneup check; fi |
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.
@sydneyjodon-wk since we're using the new "official" setup-dart GH action, you'll want to merge in the latest from the dart-2.9-boilerplate-wip
branch, and then replace stable@2.7.2
with just 2.7.2
anywhere you've used it in this PR.
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.
Updated in 5f5a492
@sydneyjodon-wk It looks like CI is failing due to outdated generated files, caused by merging in master :/ |
All feedback addressed
All feedback addressed
@greglittlefield-wf, fixed! |
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.
- Changes look good (see one last comment)
- CI runs analysis as expected for each version
- CI passes except for known, unrelated failures in dev branch related to NNBD configuration
- Examples look good
I was about to approve/QA, but I found one more small thing, sorry :(
Co-authored-by: Greg Littlefield <greg.littlefield@workiva.com>
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.
- Changes look good (see one last comment)
- CI runs analysis as expected for each version
- CI passes except for known, unrelated failures in dev branch related to NNBD configuration
- Examples look good
+10
@Workiva/release-management-p |
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.
+1 from RM
Motivation
In order to verify that there are no edge cases we missed in the new boilerplate, we need to run analysis in CI pre-build without the generated files checked in. Because pre-build analysis without generated files won't run cleanly in Dart 2.9 (because we can't ignore
undefined_identifier
errors), we can't run pre-build analysis on the whole repo so we will just run it on examples of each boilerplate.Changes
build.yaml
to build to cache.example
directory.tuneup check
for Dart 2.7 anddart analyze
for Dart >2.10 becausedartanalyzer
doesn't use the analysis server and won't see the generated files (see discussion here)Release Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
tuneup check
on justexample/boilerplate_versions
tuneup check
on the whole projectdart analyze
on the whole projectdart analyze
on justexample/boilerplate_versions/mixin_based/dart_2_9_compatible
example
and verify that the boilerplate version testing page doesn't have any errorsMerge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: