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

4-adding-workflows #5

Merged
merged 4 commits into from
Apr 4, 2024
Merged

4-adding-workflows #5

merged 4 commits into from
Apr 4, 2024

Conversation

ghost
Copy link

@ghost ghost commented Feb 8, 2024

Adding ai-cfia Workflows to the current branch to comply with our internal standard

@ghost ghost requested a review from rngadam February 8, 2024 15:18
@ghost ghost self-assigned this Feb 8, 2024
@ghost ghost added the documentation Improvements or additions to documentation label Feb 8, 2024 — with GitHub Codespaces
@ghost ghost linked an issue Feb 8, 2024 that may be closed by this pull request
@rngadam
Copy link
Contributor

rngadam commented Feb 9, 2024

the workflow should be running in this pull request. If you look at the Actions logs, you can see there is a problem with the workflow itself:

https://github.com/ai-cfia/nachet-metadata-format/actions/runs/7831834689

Copy link
Contributor

@rngadam rngadam left a comment

Choose a reason for hiding this comment

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

the workflow isn't running.

.github/workflows/workflows.yml Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Feb 15, 2024

I've figure the issue (Action secrets are missing) and will try to solve it by adding the secrets needed

@rngadam
Copy link
Contributor

rngadam commented Feb 16, 2024

Check the Actions tab of the repository for details on why the workflows do not startup:

image

@ghost
Copy link
Author

ghost commented Feb 16, 2024

The workflows are now running, however the yaml-check seems to not be working properly. I will need to investigate

Copy link
Contributor

@rngadam rngadam left a comment

Choose a reason for hiding this comment

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

quite a bit of link check failures

Copy link
Contributor

@rngadam rngadam left a comment

Choose a reason for hiding this comment

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

I see there's a couple of trailing spaces and such that need fixing.

For line length, @ThomasCardin and @SonOfLope modified the rules to allow longer lines and a number of other tweaks:

https://github.com/ai-cfia/howard/blob/main/.yamllint.yml

instead of doing per repo, I'd rather we have unified rules in ai-cfia/github-workflows for all.

@SonOfLope
Copy link
Contributor

I see there's a couple of trailing spaces and such that need fixing.

For line length, @ThomasCardin and @SonOfLope modified the rules to allow longer lines and a number of other tweaks:

https://github.com/ai-cfia/howard/blob/main/.yamllint.yml

instead of doing per repo, I'd rather we have unified rules in ai-cfia/github-workflows for all.

I will create an issue to support this. Currently we can only specify a config file from the calling repository. We can discuss the proper solution in that issue.

@rngadam
Copy link
Contributor

rngadam commented Mar 4, 2024

I will create an issue to support this. Currently we can only specify a config file from the calling repository. We can discuss the proper solution in that issue.

Yes, implicitly I'm asking for a new issue.

Also suggest you use "Reference in new issue" so we have context for that new issue (and a bidirectional link so we know that there's a follow up):

image

@SonOfLope
Copy link
Contributor

I will create an issue to support this. Currently we can only specify a config file from the calling repository. We can discuss the proper solution in that issue.

Yes, implicitly I'm asking for a new issue.

Also suggest you use "Reference in new issue" so we have context for that new issue (and a bidirectional link so we know that there's a follow up):

image

I had already created an issue and pull request. Noted for next time.
Issue : ai-cfia/github-workflows#101
PR : ai-cfia/github-workflows#102

@rngadam
Copy link
Contributor

rngadam commented Mar 4, 2024

@FrancoisWerbrouck-CFIA the now working checks are reporting some deviation from standards; please fix. Ensure that your IDE is setup to catch these issues before they are pushed to the repository.

@SonOfLope
Copy link
Contributor

@FrancoisWerbrouck-CFIA omitting a config file for the yaml linting workflow should now allow to make use of the default config from github-workflow repository.

@Francois-Werbrouck
Copy link
Contributor

I made all the workflows run and complete their default checks. I will add specifics tests in others PR.

The core of the issue #3 as been completed, which was adding the workflows to the current Repo

Copy link
Member

@ThomasCardin ThomasCardin left a comment

Choose a reason for hiding this comment

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

I see that all workflows are passing successfully. Looks good to me!

.github/workflows/requirements.txt Outdated Show resolved Hide resolved
.github/workflows/workflows.yml Outdated Show resolved Hide resolved
tests/testing_utils.py Outdated Show resolved Hide resolved
@@ -3,5 +3,5 @@
def raise_error(message):
raise Exception(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of Exception, I suggest that you use a custom Exception:

# On top of the file, or in an exception module
EnvironmentVariableError(Exception):
    pass

# Of course use the class name that you feel is best for your project

# Then: 
NACHET_SCHEMA = os.getenv("NACHET_SCHEMA") or raise EnvironmentVariableError("NACHET_SCHEMA is not set")

Copy link
Contributor

@rngadam rngadam left a comment

Choose a reason for hiding this comment

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

please address the comments from Maxence

@Francois-Werbrouck
Copy link
Contributor

Francois-Werbrouck commented Mar 27, 2024

I'm having issue with the secrets in the Action phase. The testing works image but once it goes into the Github action workflow, it fails. However I've also added the secrets into the Action' secrets
image

I'll talk with the devops guys since they just release some doc about secrets handling

@SonOfLope
Copy link
Contributor

Failing pipeline will be fixed with ai-cfia/github-workflows#110

Copy link
Contributor

@rngadam rngadam left a comment

Choose a reason for hiding this comment

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

I suggest rebasing before merging as 36 commits for a few changes is a bit excessive.

.gitignore Outdated Show resolved Hide resolved
@SonOfLope
Copy link
Contributor

Pipeline for linting python test is fixed as we can see with checks

@Francois-Werbrouck
Copy link
Contributor

I suggest rebasing before merging as 36 commits for a few changes is a bit excessive.

Done, I will now merge the branch and rebase my other branches on main

@Francois-Werbrouck Francois-Werbrouck merged commit e8b8f82 into main Apr 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Adding Github Workflows
5 participants