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.

Sign up for GitHub

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

Jump to bottom

EXP-3458: add nimbus-fml validate command #5607

Merged
merged 8 commits into from Jun 8, 2023
Merged

EXP-3458: add nimbus-fml validate command #5607

merged 8 commits into from Jun 8, 2023

Conversation

jeddai
Copy link
Member

@jeddai jeddai commented May 25, 2023 •
edited

This PR adds a command to the nimbus-fml CLI that validates an FML file and all of its channels.

EXP-3458

Pull Request checklist

  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

@jeddai jeddai added nimbus Nimbus, the experimentation platform. rust Pull requests that update Rust code labels May 25, 2023
@jhugman jhugman self-requested a review May 25, 2023 18:11
@codecov-commenter
Copy link

codecov-commenter commented May 25, 2023 •
edited

Codecov Report

Patch coverage: 95.61% and project coverage change: +0.21 🎉

Comparison is base (42c3521) 84.53% compared to head (02ef38d) 84.74%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5607      +/-   ##
==========================================
+ Coverage   84.53%   84.74%   +0.21%     
==========================================
  Files         105      105              
  Lines       11076    11179     +103     
==========================================
+ Hits         9363     9474     +111     
+ Misses       1713     1705       -8
Impacted Files Coverage Δ
components/support/nimbus-fml/src/commands.rs 80.00% <ø> (ø)
components/support/nimbus-fml/src/workflows.rs 83.29% <94.73%> (+3.18%) ⬆️
components/support/nimbus-fml/src/main.rs 90.07% <100.00%> (+0.67%) ⬆️
components/support/nimbus-fml/src/parser.rs 95.14% <100.00%> (+0.01%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

eliserichards
eliserichards approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor

@eliserichards eliserichards left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

components/support/nimbus-fml/src/main.rs Show resolved Hide resolved
components/support/nimbus-fml/src/workflows.rs Outdated Show resolved Hide resolved
jhugman
jhugman suggested changes May 30, 2023
View reviewed changes
Copy link
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

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

This is looking okay, but I don't think it's quite ready to land.

There's some complexity around imports which isn't quite working as expected. I've left some notes below.

components/support/nimbus-fml/src/parser.rs Outdated Show resolved Hide resolved
components/support/nimbus-fml/src/workflows.rs Outdated Show resolved Hide resolved
components/support/nimbus-fml/src/workflows.rs Outdated Show resolved Hide resolved
components/support/nimbus-fml/src/workflows.rs Outdated Show resolved Hide resolved
components/support/nimbus-fml/src/workflows.rs Outdated Show resolved Hide resolved
@jeddai jeddai requested a review from jhugman June 6, 2023 19:09
@jeddai jeddai force-pushed the nimbus-fml-validate-command branch from 1b80596 to 8751416 Compare June 7, 2023 13:42
@jhugman
Copy link
Contributor

jhugman commented Jun 8, 2023

Latest testing:

$ nimbus-fml validate @mozilla-mobile/firefox-android/fenix/app/nimbus.fml.yaml
ℹ️ Loaded modules:
- https://raw.githubusercontent.com/mozilla-mobile/firefox-android/main/fenix/app/onboarding.fml.yaml
- https://raw.githubusercontent.com/mozilla-mobile/firefox-android/main/fenix/app/nimbus.fml.yaml
- https://raw.githubusercontent.com/mozilla-mobile/firefox-android/main/android-components/components/browser/engine-gecko/geckoview.fml.yaml
- https://raw.githubusercontent.com/mozilla-mobile/firefox-android/main/android-components/components/service/nimbus/messaging.fml.yaml

Validating manifest for different channels:
✅ release.............valid
✅ beta................valid
✅ nightly.............valid
✅ developer...........valid

$ echo $?
0

Lovely.

$ nimbus-fml validate @mozilla-mobile/firefox-ios/nimbus.fml.yaml
Error: Property `wallpaper-feature` not found on feature `homescreenFeature`

$ echo $?
1

This is less helpful.

@jeddai
Copy link
Member Author

jeddai commented Jun 8, 2023

Oh dang I've never seen that one — is the error present in the core FML module?

jhugman
jhugman approved these changes Jun 8, 2023
View reviewed changes
Copy link
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

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

This is lovely. Thank you for iterating on the output and the colors are 👨‍🍳 kiss.

components/support/nimbus-fml/src/workflows.rs Outdated
))?;
return Ok(());
}
let intermediate_representation = parser.get_intermediate_representation(&channels[0])?;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if we can get this to fail with some indication of which file is being processed.

Or at least, output the error with an output_err.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think I'm going to add an optional manifests hashmap param to that method so that we can get the imports in order and keep a record of which ones were imported and where it failed and stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

And maybe rework the iteration so it keeps iterating but records successes/failures

Copy link
Member Author

Choose a reason for hiding this comment

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

But I'm going to handle that in a separate ticket (after updating that error to go to output_err)

Copy link
Member Author

Choose a reason for hiding this comment

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

https://mozilla-hub.atlassian.net/browse/EXP-3551

@jeddai jeddai added this pull request to the merge queue Jun 8, 2023
Merged via the queue into mozilla:main with commit e975fbe Jun 8, 2023
17 checks passed
@jeddai jeddai deleted the nimbus-fml-validate-command branch June 8, 2023 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brennie brennie brennie left review comments

@jhugman jhugman jhugman approved these changes

@eliserichards eliserichards eliserichards approved these changes

Assignees
No one assigned
Labels
nimbus Nimbus, the experimentation platform. rust Pull requests that update Rust code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jeddai @codecov-commenter @jhugman @brennie @eliserichards