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

feat: detect all package changes #1006

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SMillerDev
Copy link
Member

@SMillerDev SMillerDev commented Jan 31, 2024

This should allow us to use this flow in homebrew/cask too.

This comment was marked as resolved.

@SMillerDev SMillerDev force-pushed the feat/detect/detect_all_packages branch 5 times, most recently from 3f2a1bf to e47f98c Compare January 31, 2024 12:51
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks good so far!

I guess it feels a little half-way for now; I'd probably rather it either moves to entirely moving things over to packages or having separate formulae/casks for variables and flags. There's probably always going to warrant a need for some different classes for Formulae*/Casks*

@SMillerDev
Copy link
Member Author

Doesn't every tap use this function by default? That would be a lot of people who suddenly have their tap broken if we remove the formulae_ variables.

I'm fine splitting up the variables and/or making a combination. But just to make sure: you're only talking about this method right? You're not looking for me to add casks to all flags in this PR.

@MikeMcQuaid
Copy link
Member

Doesn't every tap use this function by default? That would be a lot of people who suddenly have their tap broken if we remove the formulae_ variables.

It does but we can/should change that in Homebrew/brew and add a deprecation.

I'm fine splitting up the variables and/or making a combination. But just to make sure: you're only talking about this method right? You're not looking for me to add casks to all flags in this PR.

I think it's fine to have for now but I guess I'm a little confused as to how it ends up being useful when only detection is done and nothing else?

@SMillerDev
Copy link
Member Author

Since detection is a separate step, it allows me to move another part of cask CI to testbot.

Everything after that will take more work so I'd like to do them separately

@MikeMcQuaid
Copy link
Member

Since detection is a separate step, it allows me to move another part of cask CI to testbot.

Ok, makes sense. If it's useful as-is: great.

cmd/test-bot.rb Show resolved Hide resolved
cmd/test-bot.rb Outdated Show resolved Hide resolved
lib/tests/packages_detect.rb Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Mar 5, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale label Mar 5, 2024
@SMillerDev SMillerDev removed the stale label Mar 5, 2024
@SMillerDev SMillerDev force-pushed the feat/detect/detect_all_packages branch from e47f98c to 985e984 Compare March 23, 2024 17:27
@SMillerDev
Copy link
Member Author

Updated to be specific to casks.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Great work, thanks @SMillerDev!

@SMillerDev SMillerDev force-pushed the feat/detect/detect_all_packages branch 5 times, most recently from 81625cd to c181e87 Compare March 25, 2024 11:00
@MikeMcQuaid
Copy link
Member

Good to 🚢 when CI is 🟢

@SMillerDev
Copy link
Member Author

I have no clue why CI could be failing, as far as I can tell it should be bright green

lib/tests/packages_detect.rb Outdated Show resolved Hide resolved
lib/tests/packages_detect.rb Show resolved Hide resolved
cmd/test-bot.rb Outdated Show resolved Hide resolved
lib/tests/packages_detect.rb Outdated Show resolved Hide resolved
@SMillerDev SMillerDev force-pushed the feat/detect/detect_all_packages branch from c181e87 to 44a86fe Compare April 18, 2024 16:33
@SMillerDev SMillerDev force-pushed the feat/detect/detect_all_packages branch from 44a86fe to 64da960 Compare April 21, 2024 09:23
@MikeMcQuaid
Copy link
Member

Invalid usage: Did not find any formulae or commits to test!

Looks like CI is perhaps legitimately broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants