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

update etreeutils.TransformExcC14n signature #478

Merged
merged 7 commits into from Jan 12, 2023

Conversation

greenpau
Copy link
Sponsor Contributor

The invocation does not match the current function signature:

github.com/crewjam/saml@v0.4.10/schema.go:767:41: not enough arguments in call to etreeutils.TransformExcC14n
     |  have (*etree.Element, string)
     |  want (*etree.Element, string, bool)

Resolves: #461

The invocation does not match the current function signature:

```
github.com/crewjam/saml@v0.4.10/schema.go:767:41: not enough arguments in call to etreeutils.TransformExcC14n
     |  have (*etree.Element, string)
     |  want (*etree.Element, string, bool)
```
@crewjam
Copy link
Owner

crewjam commented Dec 31, 2022

LGTM but looks like CI failed.

@greenpau
Copy link
Sponsor Contributor Author

@crewjam , thank you for looking in to this. I’ll fix it.

@crewjam
Copy link
Owner

crewjam commented Dec 31, 2022

See also #437.

@greenpau
Copy link
Sponsor Contributor Author

Running into the error when bumping to github.com/russellhaering/goxmldsig v1.2.0.

--- FAIL: TestSPRejectsInjectedComment (0.01s)
panic: interface conversion: error is nil, not *saml.InvalidResponseError [recovered]
        panic: interface conversion: error is nil, not *saml.InvalidResponseError

cc: #437

@greenpau
Copy link
Sponsor Contributor Author

@crewjam , please re-run the test. I got a clean test with go1.17.

@crewjam
Copy link
Owner

crewjam commented Dec 31, 2022

Done. I also need to go dig for the setting that let's CI run automatically for you, but I'm on vacation right now and left the laptop behind. :)

@greenpau
Copy link
Sponsor Contributor Author

Done. I also need to go dig for the setting that let's CI run automatically for you, but I'm on vacation right now and left the laptop behind. :)

@crewjam, nothing wrong with a healthy dose of Github while on vacation 😄

@greenpau
Copy link
Sponsor Contributor Author

@crewjam , need you to re-launch the test again. Did not tidy the go.sum before.

IceCodeNew added a commit to IceCodeNew/go-collection that referenced this pull request Jan 3, 2023
wait for crewjam/saml#478 been merged.
IceCodeNew added a commit to IceCodeNew/go-collection that referenced this pull request Jan 5, 2023
wait for crewjam/saml#478 been merged.
@IceCodeNew
Copy link
Contributor

@crewjam hate to bother you, but this PR seems been stale for a while. I am keen to see this PR getting merged.
Would you please re-run the GitHub Action at your convenience?

@crewjam
Copy link
Owner

crewjam commented Jan 9, 2023

I think the most recent one has failed. Haven't had a chance to look further

@greenpau
Copy link
Sponsor Contributor Author

greenpau commented Jan 9, 2023

@crewjam , I am running with the go1.17 test -v and go1.18 test -v locally and they both pass.

@greenpau
Copy link
Sponsor Contributor Author

@crewjam , updated to the latest github.com/russellhaering/goxmldsig with its dependencies. Please approve the workflow.

@crewjam
Copy link
Owner

crewjam commented Jan 12, 2023

Done. I did just fix the setting so this shouldn't keep needing approval.

@greenpau
Copy link
Sponsor Contributor Author

@crewjam , the test now pass. However, the linter throws an error:

run golangci-lint
  Running [/home/runner/golangci-lint-1.46.2-linux-amd64/golangci-lint run --out-format=github-actions] in [] ...
  Error: File is not `gofmt`-ed with `-s` (gofmt)
  
  Error: issues found
  Ran golangci-lint in 17892ms

@crewjam crewjam merged commit 5a6e8cc into crewjam:main Jan 12, 2023
@crewjam
Copy link
Owner

crewjam commented Jan 12, 2023

I'll fix up the linter. Thanks! And sorry this was such a slog. :/

@greenpau
Copy link
Sponsor Contributor Author

@crewjam , thank you for merging this 👍 if possible, please tag with new version.

@greenpau greenpau deleted the fix-schema branch January 12, 2023 23:49
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.

Error in schema.go
3 participants