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

DID Exchange in ACA-Py 0.7.4 with explicit invitations and without auto-accept broken #1936

Closed
dbluhm opened this issue Sep 10, 2022 · 17 comments
Assignees
Milestone

Comments

@dbluhm
Copy link
Contributor

dbluhm commented Sep 10, 2022

While working with @mepeltier on testing out a few different scenarios on ACA-Py 0.7.4, we (re)discovered that the connection state after receiving a did exchange request was not being saved, making it impossible to accept the request. This was fixed in main (and included in 1.0.0-rc0) by #1881. At the time I reviewed that PR, I did not realize how big of an issue the PR was fixing. Looking at it again now, I think it warrants a patch release.

Main has seen some changes lately that likely wouldn't fit in a patch release. With that in mind, I think cherry-picking and producing a release on a separate branch is appropriate. I'd be happy to put in the effort to prepare a branch with this state.

Should we do this? Are there other fixes that should be included in a 0.7.5 patch release?
@swcurran @ianco @andrewwhitehead

@dbluhm
Copy link
Contributor Author

dbluhm commented Sep 10, 2022

Also probably worth making sure we hit this scenario in the integration tests.

@dbluhm dbluhm self-assigned this Sep 16, 2022
@dbluhm
Copy link
Contributor Author

dbluhm commented Sep 20, 2022

I'll review PRs merged since 0.7.4 and report back whether there are any I think should also be included in a 0.7.5 patch release.

@ianco
Copy link
Contributor

ianco commented Sep 20, 2022

Also probably worth making sure we hit this scenario in the integration tests.

The integration tests run with most of the "auto" flags on, whereas AATH runs with most of the "auto" flags off

@ianco
Copy link
Contributor

ianco commented Sep 20, 2022

I'll review PRs merged since 0.7.4 and report back whether there are any I think should also be included in a 0.7.5 patch release.

This one please: #1913

@dbluhm
Copy link
Contributor Author

dbluhm commented Sep 20, 2022

The mobile team at Indicio is interested in seeing #1940 included in a patch.

@swcurran swcurran added this to the Version 0.7.5 milestone Sep 20, 2022
@swcurran
Copy link
Contributor

@ianco -- I tend to call #1913 a breaking change, so would prefer to leave it for 1.0.0. What is the rationale for putting it into 0.7.5?

@ianco
Copy link
Contributor

ianco commented Sep 20, 2022

@ianco -- I tend to call #1913 a breaking change, so would prefer to leave it for 1.0.0. What is the rationale for putting it into 0.7.5?

Northern Block has specifically asked for a release with this fix

@swcurran
Copy link
Contributor

Could we do that as a 1.0.0-rc1 for that? I'm happy to do that.

@ianco
Copy link
Contributor

ianco commented Sep 20, 2022

Could we do that as a 1.0.0-rc1 for that? I'm happy to do that.

For sure I think that would be fine.

@Durai46
Copy link

Durai46 commented Sep 27, 2022

@swcurran Are you going to publish the docker image for the same?

@swcurran
Copy link
Contributor

Yes. Completing the 0.7.5 release is still a in progress -- there is at least one more PR to go. Would it help you for me to produce a 1.0.0-rc1, or do you need the 0.7.5-rc0/0.7.5 image?

Sorry for the delay.

@Durai46
Copy link

Durai46 commented Oct 3, 2022

Thank you @swcurran I would go for 0.7.5 image.

@swcurran
Copy link
Contributor

Tag and image 0.7.5-rc0 exists and seems to be fine. @Durai46 and @dbluhm -- can you confirm we are good to go with "0.7.5" final? I think it is an easy one, but just wanted to get this finished.

We've run AATH tests and things are working with one odd, but likely unrelated bit of weirdness. If you want the details I can provide, but it's convoluted! :-(

Once I get a thumbs up I'll do a final PR for the 0.7.5 release and we'll count it done.

@dbluhm
Copy link
Contributor Author

dbluhm commented Oct 13, 2022

I haven't had a chance to give it the full battery of tests we like to run (basically just plugging into our plugins/projects and running integration tests) but I have run a few and it's looking good so far. Critically, the original problem I opened this issue for I have tested and found working in 0.7.5-rc0.

@swcurran
Copy link
Contributor

OK -- Glad to hear the main issue is addressed. I'm doing a bit more and we'll make a call that it is done Real Soon Now. Please let me know if you do more and the results are good/bad.

Thanks!

@swcurran
Copy link
Contributor

FYI -- the "weirdness" with AATH has been solved. Weird answer, but now working... :-)

@dbluhm
Copy link
Contributor Author

dbluhm commented Nov 3, 2022

With 0.7.5 released, closing this issue 🙂

@dbluhm dbluhm closed this as completed Nov 3, 2022
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

No branches or pull requests

4 participants