-
Notifications
You must be signed in to change notification settings - Fork 448
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
Sgmoore/add core24 support #5092
base: main
Are you sure you want to change the base?
Sgmoore/add core24 support #5092
Conversation
didn't make it into my local repo.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5092 +/- ##
==========================================
- Coverage 94.88% 89.71% -5.17%
==========================================
Files 658 341 -317
Lines 55189 22649 -32540
==========================================
- Hits 52364 20320 -32044
+ Misses 2825 2329 -496 ☔ View full report in Codecov by Sentry. |
@mr-cal @sergiusens Please help with this security scan. I don't know how to fix it. Thanks |
Hi @ScarlettGatelyMoore, you can ignore the security scan failures for now, it is a problem between github and trivy that @lengau has been investigating. This is the error I'm referring to:
|
@mr-cal @sergiusens I believe the security scan is fixed with this, but I haven't properly tested it yet so I don't have a PR. ITMT we can just re-run them. |
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 LGTM so far, but I'd like to snap an app with it before I give explicit approval just in case I come across any quirks we might want to document.
"default-provider": "mesa-2404", | ||
}, | ||
} | ||
gpu_layouts = { |
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 isn't really about this PR, but comparing this to the gnome extension we've got a lot of overlap. I wonder whether it would be worth creating an abstract DesktopExtension where this sort of general stuff could live, so we can share more common work?
@mr-cal @sergiusens @kenvandine what do you think?
Things that overlap and could just be extended for each desktop include
- plugs
- GPU plugs
- GPU layouts
- Some base themes
I'm happy to do the work for that, but I only want to do it if I'm actually making things better :-)
I can't seem to sort this one out - google:ubuntu-22.04-64:tests/spread/core22/linters/library |
There's nothing useful in the log. I'll run it again and see if it fails again. |
It looks like last rebase fixed it. |
@ScarlettGatelyMoore - is this ready for review? |
Yes... please. |
|
I will review this week |
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.
Looks good, just some minor feedback.
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.
LGTM! I installed this branch and built a Keysmith snap off it, which I intend to make a merge request for as soon as this lands on stable.
Improve error. Co-authored-by: Alex Lowe <alex.lowe@canonical.com>
Done! Thanks! |
Well it's broken now after @lengau error change. I am confused as to why. I can't find where the call is coming from and our snaps have worked just fine. I have added the file it is demanding to the content snap, but it will be awhile before it will be done. This makes zero sense to me. |
tox run -m lint
?tox run -e test-py310
? (supported versions:py39
,py310
,py311
,py312
)