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

chore(deps): upgrade angular in examples/supply chain app #2020

Merged

Conversation

aldousalvarez
Copy link
Contributor

Fixes #2002

Signed-off-by: aldousalvarez [email protected]

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@aldousalvarez LGTM, thank you!

Copy link
Contributor

@izuru0 izuru0 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@aldousalvarez Unfortunately a merge conflict came in through one of the commits that got merged to main since this was approved. Please fix the conflict and then we should be good to go again.

Don't forget to re-request review when done.

@aldousalvarez aldousalvarez force-pushed the aldousalvarez/issue2002 branch 3 times, most recently from 56e6206 to e6055db Compare May 17, 2022 01:20
@aldousalvarez
Copy link
Contributor Author

Hello @petermetz fixed the merge conflict that you said. And now all checks have passed. Thank you

Copy link
Contributor

@jagpreetsinghsasan jagpreetsinghsasan left a comment

Choose a reason for hiding this comment

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

LGTM

@aldousalvarez aldousalvarez force-pushed the aldousalvarez/issue2002 branch 3 times, most recently from 7d40669 to f4ce927 Compare May 24, 2022 01:22
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

Hello @petermetz fixed the merge conflict that you said. And now all checks have passed. Thank you

@aldousalvarez Great, thanks. Please rebase onto upstream/main and then run yarn to make sure that the lock file is updated. Right now if you run yarn it produces a diff on the lock file.

@aldousalvarez
Copy link
Contributor Author

@petermetz Already done. Thank you

@aldousalvarez
Copy link
Contributor Author

Depends on #2096

@aldousalvarez aldousalvarez force-pushed the aldousalvarez/issue2002 branch 3 times, most recently from 62ed9a2 to 3892c83 Compare July 13, 2022 01:48
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@aldousalvarez Please fix the merge conflicts and then LGTM.

@petermetz
Copy link
Contributor

@aldousalvarez Please put the dependency declaration to the PR description next time. The bot doesn't scan the PR comments for it.

@aldousalvarez aldousalvarez force-pushed the aldousalvarez/issue2002 branch 2 times, most recently from 3d378d1 to 77ff220 Compare July 14, 2022 01:07
@aldousalvarez
Copy link
Contributor Author

@aldousalvarez Please fix the merge conflicts and then LGTM.

@petermetz Fixed the merge conflicts and now all checks have passed.

@aldousalvarez Please put the dependency declaration to the PR description next time. The bot doesn't scan the PR comments for it.

My apologies on this one. Should I put it on the commit message with the fixes and signed-off-by?

@petermetz
Copy link
Contributor

@aldousalvarez It still shows that the conflicts are there =>

This branch has conflicts that must be resolved
 to resolve conflicts before continuing.
Conflicting files
yarn.lock

@petermetz
Copy link
Contributor

My apologies on this one. Should I put it on the commit message with the fixes and signed-off-by?

@aldousalvarez Yes, that works best. I usually have the Fixes line and then right below it the depends on line

@aldousalvarez aldousalvarez force-pushed the aldousalvarez/issue2002 branch 2 times, most recently from 3e3198a to 7136174 Compare July 15, 2022 03:48
@aldousalvarez
Copy link
Contributor Author

@aldousalvarez It still shows that the conflicts are there =>

This branch has conflicts that must be resolved
 to resolve conflicts before continuing.
Conflicting files
yarn.lock

my apologies on that one,

as of now all checks have passed and all conflicts have been resolved

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@aldousalvarez No worries, thanks for fixing it!
Other advice for the future (also not a big deal at all, just making sure you are aware) =>
when you make a change in response to a change request review (like the conflict resolution you just did) then you have to "pass the ball back" to the reviewer by re-requesting the review when you've addressed the change request.

@petermetz petermetz merged commit f328dd8 into hyperledger-cacti:main Jul 16, 2022
@petermetz petermetz deleted the aldousalvarez/issue2002 branch July 16, 2022 17:25
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.

chore(deps): upgrade angular in examples/supply chain app
4 participants