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.

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

chore: purge all the default container configuration #1901

Merged
merged 5 commits into from
Nov 23, 2024

Conversation

tulilirockz
Copy link
Collaborator

@tulilirockz tulilirockz commented Nov 6, 2024

I believe these are all the references to the Bluefin-CLI container. Probably should not be merged right now? Maybe disabiling the service files then deleting the references seems like the best approach, as Jorge said.

The updated title is just a joke! We talked about this on discord and it seems like it would be best to just get rid of the default containers and stuff

From Jorge:

I'm thinking total purge -
I'd rather spend more time removing as much as we can and then documenting what we have, lean and mean.
the quadlets are in the toolbox repos already

Affects stuff like #1834 and #1881

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Nov 6, 2024
@castrojo
Copy link
Member

castrojo commented Nov 6, 2024

So I don't mean to hijack this PR. But, looks like the profile setup for ptyxis changed, and now that we have more users people are getting confused.

It might be worth to just rip all of these out and just ship distrobox/boxbuddy blank out of the box:

  • our images are listed in boxbuddy now anyway
  • new users: less footguns
  • pro users: know how to container

@tulilirockz tulilirockz changed the title chore: remove references to deprecated bluefin-cli container chore: PURGE all the containers out of BLUEFIN 😈😈😈 Nov 6, 2024
@tulilirockz tulilirockz marked this pull request as draft November 6, 2024 22:55
@tulilirockz
Copy link
Collaborator Author

Just converted this into a draft cuz this PR seems kinda big and probably needs a lot more confirmation from everyone

@tulilirockz
Copy link
Collaborator Author

I agressively just removed everything related to those default containers, please tell me if you guys dont want something removed!!

Im just wondering about the podmansh.container file in /usr/share/ublue-os/quadlet and /usr/bin/bluefinbox-enter. Should those be removed too?

@tulilirockz tulilirockz marked this pull request as ready for review November 7, 2024 02:57
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Nov 7, 2024
@tulilirockz
Copy link
Collaborator Author

After testing it a bit, this seems fine? Nothing seems to have broken, nothing seems to have changed majorly, my vm seems completely ok and happy

@tulilirockz
Copy link
Collaborator Author

Although that ptyxis-create-profile command doesnt seem to be working too well if Ptyxis's dconf keys get reset. It just errors out trying to set transparency since it cant read the default profile uuid off of dconf

@tulilirockz tulilirockz changed the title chore: PURGE all the containers out of BLUEFIN 😈😈😈 chore: purge all the default container configuration Nov 7, 2024
@castrojo
Copy link
Member

I think we can land this after the conflicts get resolved, and then I'll remove this section from the docs, thanks!

@tulilirockz
Copy link
Collaborator Author

I think we can land this after the conflicts get resolved, and then I'll remove this section from the docs, thanks!

Oh! I didnt even like see the conflicts here LOL i was just editing stuff directly on the my local copy. Sorry! Ill resolve them rn

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Nov 10, 2024
@tulilirockz
Copy link
Collaborator Author

Hopefully this should be good now

@lethedata
Copy link
Contributor

lethedata commented Nov 12, 2024

Are the containers themselves being depreciated or just the bluefin default integration? If it's just the default integration could this be left in the dx version or convert setup to ujust scripts?

I was a bit caught of guard when #1908 ripped out the bluefin-cli after an updated and it's a but unclear if the container's being depreciated or just the bluefin integration.

@m2Giles
Copy link
Member

m2Giles commented Nov 16, 2024

What's specifically being deprecated is the ephemeral containers that we shipped on ctrl+alt+f and ctrl+alt+u (fedora/ubuntu)

Copy link
Member

@m2Giles m2Giles left a comment

Choose a reason for hiding this comment

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

The only other thought I have, is that the user's targets for the quadlets are going to just still be there. I'm hesitant to auto clean them up in case someone goes the same route for enabling them.

Additionally, we should probably update the distrobox.ini files in toolboxes in case people want to consume with distrobox's ability to use assemble with a url now.

Specifically, there were targets for the following:
ubuntu-toolbox.target
fedora-toolbox.target
bluefin-cli.target
wolfi-toolbox.target
bluefin-cli-dx.target
wolfi-dx-toolbox.target

@befanyt
Copy link
Contributor

befanyt commented Nov 17, 2024

@tulilirockz I noticed that after your branch updates it seems to be missing the custom2 vs custom3 in the keybindings again. Just to let you know ❤️

@tulilirockz
Copy link
Collaborator Author

@tulilirockz I noticed that after your branch updates it seems to be missing the custom2 vs custom3 in the keybindings again. Just to let you know ❤️

oh great. :p

@befanyt
Copy link
Contributor

befanyt commented Nov 17, 2024

@tulilirockz I noticed that after your branch updates it seems to be missing the custom2 vs custom3 in the keybindings again. Just to let you know ❤️

oh great. :p

You applied the commit via my review, that is probably why the commit is missing now. Probably need to commit it again by yourself

@tulilirockz tulilirockz force-pushed the remove-bluefin-cli-container branch 2 times, most recently from c3f3acd to 7528f24 Compare November 17, 2024 17:39
@m2Giles
Copy link
Member

m2Giles commented Nov 18, 2024

This is looking good. My only hesitations are cleaning up the old targets and using the name Host for the profile.

Instead I think all of the ptyxis profile manipulation can be ripped out. You should be able to use uuid for the shortcut and it will then just open whatever that profile is configured with.

@tulilirockz
Copy link
Collaborator Author

We also shoukd change the build script orders too since the 06 script got removed - I dont think this should be done in this PR since itll bring merge conflicts unnecessarily AFAIK

@tulilirockz
Copy link
Collaborator Author

tulilirockz commented Nov 19, 2024

This is looking good. My only hesitations are cleaning up the old targets and using the name Host for the profile.

Instead I think all of the ptyxis profile manipulation can be ripped out. You should be able to use uuid for the shortcut and it will then just open whatever that profile is configured with.

Do you think that should be done here? It does bring out the scope a bit - still, great idea! Cleaning up the old targets dont seem like such a huge problem, as this is mostly targeted at new systems that dont have those targets yet, if someone actually notices that those are running, they can remove/disable them on their own I think.

auto-merge was automatically disabled November 22, 2024 18:31

Head branch was pushed to by a user without write access

@tulilirockz
Copy link
Collaborator Author

sorry thought rebasing wouldnt disable auto-merge

@castrojo castrojo added this pull request to the merge queue Nov 23, 2024
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 23, 2024
github-merge-queue bot pushed a commit that referenced this pull request Nov 23, 2024
Merged via the queue into ublue-os:main with commit 0379b34 Nov 23, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants