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

Fix (FE2): CNX-8776 get header from front end #3164

Merged
merged 9 commits into from
Feb 6, 2024
Merged

Fix (FE2): CNX-8776 get header from front end #3164

merged 9 commits into from
Feb 6, 2024

Conversation

oguzhankoral
Copy link
Member

I've been following up issue that retrieving x-speckle-header-2 header from serverUrl. Here my findings,

  • We were trying to query /graphql for header but actually server doesn't know anything about this header.
  • I've changed to way of retrieving it on IsFrontend2Server function by pinging /. @jedd I'm having PR onto your branch/PR for changes.
  • Now some unit tests are failing because local server localhost:3000 doesn't proxy to any frontend to ping and get header from it. This is also a bad sign for mutating ServerInfo from different URL requests.. I've added unit tests for real servers like https://app.speckle.systems/, https://speckle.xyz/. but just in case we need to do proper testing with some alpha version when the time comes

@oguzhankoral oguzhankoral force-pushed the oguzhan/CNX-8776-Move-IsFrontend2-logic branch from c97b4bc to 13b7108 Compare February 5, 2024 14:43
AlanRynne
AlanRynne previously approved these changes Feb 6, 2024
View reviewed changes
Copy link
Member

@AlanRynne AlanRynne left a comment

Choose a reason for hiding this comment

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

So in essence we're rolling back isFrontend2 implementation to the way it used to be 🤔 is this correct?

Core/Tests/Speckle.Core.Tests.Integration/Credentials/UserServerInfoTests.cs Show resolved Hide resolved
@AlanRynne AlanRynne self-requested a review February 6, 2024 11:59
@AlanRynne AlanRynne dismissed their stale review February 6, 2024 11:59

Should have been a comment 🙇🏼‍♂️

JR-Morgan
JR-Morgan requested changes Feb 6, 2024
View reviewed changes
Core/Core/Credentials/AccountManager.cs Outdated Show resolved Hide resolved
Core/Core/Credentials/AccountManager.cs Outdated Show resolved Hide resolved
Core/Core/Credentials/AccountManager.cs Outdated Show resolved Hide resolved
@JR-Morgan JR-Morgan changed the base branch from CNX-8776-Move-IsFrontend2-logic to dev February 6, 2024 16:20
@JR-Morgan JR-Morgan mentioned this pull request Feb 6, 2024
Closed
@AlanRynne AlanRynne merged commit e1f8c00 into dev Feb 6, 2024
32 checks passed
@AlanRynne AlanRynne deleted the oguzhan/CNX-8776-Move-IsFrontend2-logic branch February 6, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlanRynne AlanRynne AlanRynne approved these changes

@JR-Morgan JR-Morgan JR-Morgan approved these changes

Assignees

@JR-Morgan JR-Morgan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@oguzhankoral @AlanRynne @JR-Morgan