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

Allow to specify a custom shepherd endpoint #9112

Conversation

alies-dev
Copy link
Contributor

Currently it's possible to specify only a custom domain, so you have to use hooks/psalm/ route. In this PR I made it more flexible, so a user can specify any URL.

Before this PR, it's possible to specify shepherd host using 2 ways (by priority):

  1. --shepherd cli option. Without value it just a flag to send report to http://shepherd.dev. It's also possible to use it with value: --shepherd=custom.domain
  2. Using a combination of 2 env vars: PSALM_SHEPHERD and PSALM_SHEPHERD_HOST. Where PSALM_SHEPHERD is a flag, using PSALM_SHEPHERD_HOST you can specify a domain (introduced by Add the environment variable for using Shepherd #5512, shipped within 4.9.0)

I think it will be nice to use a single env variable instead 2, as we do for cli option (for consistency). Also, any van names that includes "host" are not valid anymore as we can specify full URL/endpoint.

@alies-dev
Copy link
Contributor Author

ping @ngmy also. What do you think about these changes?

@alies-dev
Copy link
Contributor Author

Hey @orklah

Do you know why circleci almost always fails with Uncaught InvalidArgumentException: Could not get class storage for sebastianbergmann\objectenumerator\invalidargumentexception? Can it be a memory issue? If so - can we use GitHub actions instead?

}

return '';
/** @psalm-suppress MixedAssignment */
Copy link
Contributor Author

@alies-dev alies-dev Jan 14, 2023

Choose a reason for hiding this comment

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

this method exist just to minimise possible BC (it's public static method :/ )

@orklah
Copy link
Collaborator

orklah commented Jan 15, 2023

Can we make it so the current property doesn't change value and behaves like before until it's removed? I advised at least one person to change the value here and I wouldn't want to break that if it's not needed

Do you know why circleci almost always fails

Something is broken, it's not just a memory thing. As far as I can tell, it did not begin on a specific commit, rather at a specific time. Also the code we're analyzing did not change (we analyze a fork of phpunit). My best guess is that a compatibility layer was installed on a dependency of phpunit that makes Psalm think a class has been analyzed when it didn't.

I can't find the time to debug that unfortunately

@alies-dev
Copy link
Contributor Author

alies-dev commented Jan 16, 2023

@orklah

Can we make it so the current property doesn't change value and behaves like before until it's removed? I advised at least one person to change the value here and I wouldn't want to break that if it's not needed

Yes, we can, but it requires to write high complexity code, but I can do that. Then, for Psalm 6, we need to cleanup this complexity and it's better to do it with this (create a new PR for Psalm 6, but I don't see a branch for it)

UPD: done, I did my best, this is the weirdest peace of code I've written for latest few years, but it should work :). I would suggest to privatize more properties, classes to make such changes easier to change. By this moment a lot of Psalm code is a subject for breaking compatibility.

@orklah
Copy link
Collaborator

orklah commented Jan 18, 2023

Seems good to merge, can you rebase to solve the conflict please?

@alies-dev
Copy link
Contributor Author

@orklah
sorry, there is some noise on this PR (rebased changes are also tracked here). I have just created a clean version of this R with the same code: #9133

@alies-dev alies-dev closed this Jan 18, 2023
@alies-dev alies-dev deleted the allow-to-specify-custom-shepherd-endpoint branch February 2, 2023 20:44
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

6 participants