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

feat: add argo-lint and install-argo-cli action #171

Merged
merged 10 commits into from
Jul 11, 2024
Merged

Conversation

Duologic
Copy link
Member

@Duologic Duologic commented Jul 3, 2024

Figured this could be an interesting shared workflow to get my feet wet in this repo.

On review, this PR splits out the installation into a separate workflow that is shared with the trigger-argo-workflow action.

Changes in this PR:

  • Split installation of argo-cli into separate install-argo-cli action
  • Update trigger-argo-workflow action using above
  • Introduce new argo-lint action

@Duologic Duologic requested a review from a team as a code owner July 3, 2024 12:47
Copy link
Member

@iainlane iainlane left a comment

Choose a reason for hiding this comment

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

I think we should be able to use a path on the system instead of an artifact

actions/argo-lint/action.yaml Outdated Show resolved Hide resolved
@Duologic
Copy link
Member Author

Duologic commented Jul 4, 2024

Tested internally

@Duologic Duologic requested a review from iainlane July 4, 2024 09:09
@Duologic Duologic changed the title feat: add argo-lint action feat: add argo-lint and install-argo-cli action Jul 4, 2024
Copy link
Contributor

@dsotirakis dsotirakis left a comment

Choose a reason for hiding this comment

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

after Iain's comments and fixes, lgtm!

actions/trigger-argo-workflow/action.yaml Outdated Show resolved Hide resolved
actions/argo-lint/action.yaml Outdated Show resolved Hide resolved
@Duologic Duologic force-pushed the duologic/argo-lint branch from e9a80d1 to 0ce83cb Compare July 4, 2024 11:25
@Duologic Duologic requested a review from iainlane July 4, 2024 11:26
@Duologic Duologic force-pushed the duologic/argo-lint branch from 0ce83cb to 11c7fba Compare July 4, 2024 11:28
@Duologic
Copy link
Member Author

Duologic commented Jul 4, 2024

Comment on lines 25 to 27
# substitute your-action with a unique name (within `shared-repos` for your
# action), so if multiple actions check `shared-workflows` out, they don't
# overwrite each other
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# substitute your-action with a unique name (within `shared-repos` for your
# action), so if multiple actions check `shared-workflows` out, they don't
# overwrite each other

ARGO_VERSION=3.5.1
echo "version=${ARGO_VERSION}" >> ${GITHUB_OUTPUT}
- name: Install argo-cli
uses: grafana/shared-workflows/actions/install-argo-cli@main
Copy link
Member

Choose a reason for hiding this comment

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

this needs the same change

@Duologic Duologic force-pushed the duologic/argo-lint branch from 11c7fba to 76a765e Compare July 10, 2024 21:25
@Duologic Duologic requested review from iainlane and dsotirakis July 10, 2024 21:29
@Duologic
Copy link
Member Author

Please have another look at this, I think I've covered all points.

Copy link
Member

@iainlane iainlane left a comment

Choose a reason for hiding this comment

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

it looks good to me 👍 nice one!

one way to verify would be to trigger a run with shared-workflows@duologic/argo-lint, see if it works.

@Duologic
Copy link
Member Author

Duologic commented Jul 11, 2024

It's running internally

@Duologic Duologic merged commit d848da2 into main Jul 11, 2024
7 checks passed
@Duologic Duologic deleted the duologic/argo-lint branch July 11, 2024 09:13
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.

3 participants