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

Make examples folders self-contained by including necessary setup files #225

Closed

Conversation

alesgenova
Copy link
Collaborator

@alesgenova alesgenova commented Mar 23, 2022

Working towards #61

This change is Reviewable

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Reviewed 45 of 45 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, platform LGTM missing (waiting on @alesgenova)


.github/workflows/ci.yml, line 93 at r1 (raw file):

        run: ./scripts/continuous_integration/github_actions/ubuntu_apt/build_test
        shell: bash
  verify_setup_files:

This test shows as passing when it was run on this PR but it is actually failing with git: command not found


scripts/setup/utils/update_setup_files.sh, line 1 at r1 (raw file):

#!/bin/bash

Would it make sense to have a few lines of documentation on how/when to use this file?

Copy link
Collaborator Author

@alesgenova alesgenova left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, platform LGTM missing (waiting on @BetsyMcPhail)


.github/workflows/ci.yml, line 93 at r1 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

This test shows as passing when it was run on this PR but it is actually failing with git: command not found

I made changes to ensure that the action will actually fail if an error occurs (and also fixed the error that was happening)


scripts/setup/utils/update_setup_files.sh, line 1 at r1 (raw file):

Previously, BetsyMcPhail (Betsy McPhail) wrote…

Would it make sense to have a few lines of documentation on how/when to use this file?

Added a short explanation of what the script does

Copy link
Contributor

@BetsyMcPhail BetsyMcPhail left a comment

Choose a reason for hiding this comment

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

Verified that the test ran successfully. :lgtm:

+@jwnimmer-tri for review please

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @jwnimmer-tri)

@alesgenova alesgenova force-pushed the self-contained-examples branch 2 times, most recently from 079aec8 to ddedaec Compare March 30, 2022 20:22
@jwnimmer-tri
Copy link
Contributor

Is this PR fully finished? My recollection from the last Kitware/TRI sync meeting was that it was known to be incomplete and is still being reworked. Let me know whether I should start my review.

@jwnimmer-tri
Copy link
Contributor

Hearing nothing, I'll de-assign myself in order to clean up my reviews dashboard. Feel free to re-add me once it's ready.

@jwnimmer-tri jwnimmer-tri removed their assignment Apr 18, 2022
@github-actions
Copy link

Thank you for your contribution. This pull request has been open for 180 days without activity and so is considered stale. It will be automatically closed in 14 days unless you comment or remove the "result: stale" label.

@github-actions
Copy link

Thank you for your contribution. This pull request has been open for 180 days without activity and so is considered stale. It will be automatically closed in 14 days unless you comment or remove the "result: stale" label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants