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

Update tools/essence-feature-usage-stats #51

Closed
wants to merge 24 commits into from

Conversation

gskorokhod
Copy link
Contributor

@gskorokhod gskorokhod commented Nov 6, 2023

Done

  • Update to work with the new directory structure in conjure v2.5.1
  • Specify multiple repos for Essence files
  • Make repos sortable / hideable
  • Add environment variable to cap the number of Essence files that get processed
  • Add a way to exclude paths that match a given regex (By default, exclude all autogen files)
  • Switched to data tables plugin for the table sorting features
  • Added exporting data as CSV

This tool should be done and ready for review now!

@gskorokhod gskorokhod marked this pull request as draft November 6, 2023 19:45
@ozgurakgun
Copy link
Contributor

@gskorokhod merged main one more time to bring in the new github actions changes, will stop messing up your PR now :)

@gskorokhod gskorokhod marked this pull request as ready for review November 17, 2023 12:12
@gskorokhod
Copy link
Contributor Author

Good afternoon! I have fixed the sorting issues by switching to the DataTables plugin instead of my own sorting code, and added a link to download data as a CSV file. There is no Python lint errors and I've documented the new env variables in the project's README so should be ready to merge!
(Checks fail because it says it doesn't have permissions to deploy - The page builds fine in my fork so i assume it will be fine once this is merged?)

@gskorokhod
Copy link
Contributor Author

@ozgurakgun I'm marking this as ready for review!

@niklasdewally
Copy link
Collaborator

niklasdewally commented Nov 17, 2023

(Checks fail because it says it doesn't have permissions to deploy - The page builds fine in my fork so i assume it will be fine once this is merged?)

This is not related to your changes, dw.


Ping: @ozgurakgun

Workflows don't get write access on PRs for security.

on:
  push:
    branches:
      - main # run for pushes to the main branch
  workflow_dispatch:
  pull_request:
    paths:
      - tools/essence-feature-usage-stats/**

I would remove PR from the above in the CI file.

Alternatively, if you want this to run in PRs successfully, you can use a workflow_run rule to do this.

In short, workflow A runs in the context of the PR, generates all the pages, and stores them as artifacts.
Workflow B runs in the context of the head of main, and is triggered when workflow A finishes. It loads the artifact saved by A, and deploys it to GH pages. As workflow B runs on main, it has write permissions.

See code-coverage / code-coverage-deploy for existing examples of how to do this.

@niklasdewally
Copy link
Collaborator

The above basically ensures that a malicious contributor does not edit CI in a PR, and that edited CI nukes the repo.
Workflow B always runs the copy of itself on main, so is safe.

@niklasdewally niklasdewally changed the title (WIP) Update tools/essence-feature-usage-stats Update tools/essence-feature-usage-stats Nov 17, 2023
@ozgurakgun
Copy link
Contributor

We still want to build for PRs, but not push/deploy. An if statement to the deploy step should solve it?

@gskorokhod
Copy link
Contributor Author

We still want to build for PRs, but not push/deploy. An if statement to the deploy step should solve it?

I will have a look!

@gskorokhod
Copy link
Contributor Author

Added the if statement
By the way, why do we want to build on pull requests if it is not deployed? Is it just to ensure that it still builds correctly?

…n runs, and we don't want to clutter the repo)
@ozgurakgun
Copy link
Contributor

Added the if statement By the way, why do we want to build on pull requests if it is not deployed? Is it just to ensure that it still builds correctly?

exactly!

@@ -12,9 +12,13 @@ on:
env:
ESSENCE_DIR: "./EssenceCatalog"
Copy link
Contributor

Choose a reason for hiding this comment

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

does this still need to be in the ENV? did you do multiple repos yet in this PR?

Copy link
Contributor Author

@gskorokhod gskorokhod Nov 17, 2023

Choose a reason for hiding this comment

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

Yes! Repos are specified in another variable (ESSENCE_FILE_REPOS I think?)
This is just the root directory for essence files, all repos will be cloned to its sub directories

Copy link
Contributor

Choose a reason for hiding this comment

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

is it not a bit confusing to have it set to EssenceCatalog, which is one of the repo names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, changed that

@gskorokhod
Copy link
Contributor Author

I'm rearranging my branches (making main an exact copy of the main in conjure-cp/conjure-oxide, and creating dev branches for every feature I'm working on), so I will have to close and re-open this PR from a different branch

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