Skip to content

Error reporting for diagnostics#98

Merged
push-pull-deploy merged 5 commits intomasterfrom
error_messages
Oct 9, 2014
Merged

Error reporting for diagnostics#98
push-pull-deploy merged 5 commits intomasterfrom
error_messages

Conversation

@christianbpedersen
Copy link
Copy Markdown

Status

READY

Description

This adds some error output.

Todos

  • Write tests

Deploy Notes

Steps to Test or Reproduce

bundle exec rake

Impacted Areas in Application

@Lordnibbler
Copy link
Copy Markdown
Contributor

@christianbpedersen can you please rebase this branch off master and add tests for new code?

@push-pull-deploy
Copy link
Copy Markdown
Contributor

I've rebased off current master and will look into tests, @Lordnibbler.

@push-pull-deploy
Copy link
Copy Markdown
Contributor

@pitbulk, @luisvm, @Lordnibbler, rebased and tests added.

@push-pull-deploy
Copy link
Copy Markdown
Contributor

Oops, look like I missed two cases. Adding now...

@Lordnibbler
Copy link
Copy Markdown
Contributor

@pwnetrationguru can you please remove the version bump from this branch?

Comment thread lib/onelogin/ruby-saml/version.rb Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pwnetrationguru as per @Lordnibbler, could you please remove the version bump?

@pitbulk
Copy link
Copy Markdown
Collaborator

pitbulk commented Oct 9, 2014

@pwnetrationguru , we need that you remove version bump, also we have the #147 that includes a new error, can you mix this PR (branch issuer) with your code and add its test?

@push-pull-deploy
Copy link
Copy Markdown
Contributor

hey all, yep, i'll plan on updating this PR tomorrow! 👍

@push-pull-deploy
Copy link
Copy Markdown
Contributor

@Lordnibbler, @luisvm and @pitbulk, version bump removed, test added, should be GTG 🐥

@push-pull-deploy
Copy link
Copy Markdown
Contributor

Oops, looking at #147 now and will add that error

@push-pull-deploy
Copy link
Copy Markdown
Contributor

@Lordnibbler, @luisvm and @pitbulk, re #147. I think the best approach is merge this PR into master and then comment on #147 to add the errors to @errors

Otherwise, I'm merging in changes from a PR that hasn't been merged into master yet,etc.

Thoughts?

@pitbulk
Copy link
Copy Markdown
Collaborator

pitbulk commented Oct 9, 2014

@pwnetrationguru You can download the https://github.com/onelogin/ruby-saml/pull/147.patch do a:

git apply 147.patch

and handle with the error issue, then you can close the #147 referencing this PR, then merge this PR and at the end, delete the issuer branch

@push-pull-deploy
Copy link
Copy Markdown
Contributor

That doesn't seem to follow git best practices. What makes the most sense, to me, is get this merged into master. Then I can checkout the branch associated with #147 and add the @errors object. My suggestion would be:

  1. Merge this PR into master
  2. Rob opens PR against Fix LogoutResponse issuer validation and implement SAML Response issuer validation. Related to Pull Request 116 #147 to add @errors
  3. Merge Fix LogoutResponse issuer validation and implement SAML Response issuer validation. Related to Pull Request 116 #147 into master

@Lordnibbler
Copy link
Copy Markdown
Contributor

👍, id recommend @pwnetrationguru's steps for git but @pitbulk's will also work

push-pull-deploy pushed a commit that referenced this pull request Oct 9, 2014
Error reporting for diagnostics
@push-pull-deploy push-pull-deploy merged commit 7488f28 into master Oct 9, 2014
@push-pull-deploy push-pull-deploy deleted the error_messages branch October 9, 2014 21:02
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.

5 participants