Skip to content

Conversation

@vlussenburg
Copy link
Contributor

No description provided.

@vlussenburg vlussenburg marked this pull request as ready for review March 2, 2023 19:15
@Andrew5194
Copy link
Contributor

Andrew5194 commented Mar 2, 2023

A few things upon review:

  1. We need to rebuild using make dist-rebuild right? I don't see the related artifacts that get updated after rebuilding, see https://github.com/ForAllSecure/mcode-action#how-to-contribute <-- I would assume after we merge in Add coverage junit input runid output #45
  2. We should test that this works (I see the comment about adding the integration tests)
  3. We should test that the new release works (i.e. ForAllSecure/mcode-action@<SHA>)

@vlussenburg
Copy link
Contributor Author

A few things upon review:

  1. We need to rebuild using make dist-rebuild right? I don't see the related artifacts that get updated after rebuilding, see https://github.com/ForAllSecure/mcode-action#how-to-contribute <-- I would assume after we merge in Add coverage junit input runid output #45
  2. We should test that this works (I see the comment about adding the integration tests)
  3. We should test that the new release works (i.e. ForAllSecure/mcode-action@<SHA>)

Good point. 1. is done. 2. added a test. 3. need to do

@vlussenburg
Copy link
Contributor Author

  1. We should test that this works (I see the comment about adding the integration tests)

Works. Verified manually. --owner vlussenburg instead of forallsecure

@vlussenburg
Copy link
Contributor Author

  1. We should test that the new release works (i.e. ForAllSecure/mcode-action@)

@Andrew5194 I would argue that the integration tests do exactly that and there's no need to do it from an external repo. WDYT?

Copy link
Contributor

@unionfindbee unionfindbee left a comment

Choose a reason for hiding this comment

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

Looks good!

@vlussenburg vlussenburg merged commit 100a6f1 into main Mar 3, 2023
@vlussenburg vlussenburg deleted the input-for-owner branch March 3, 2023 17:58
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.

4 participants