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

Testing: git push auto fixed files #4190

Merged
merged 16 commits into from
Nov 1, 2024

Conversation

lukelloyd1985
Copy link
Contributor

@lukelloyd1985 lukelloyd1985 commented Oct 25, 2024

Following on from PRs #4176 & #4180 & #4183
Added git config commands for setting author
Coloured error messages in reporter if git fails for auto fixed files

Proposed Changes

Testing git add, commit & push when there are fixed sources files

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@lukelloyd1985
Copy link
Contributor Author

@nvuillam, I've moved away from messing with MegaLinter output status and just put coloured error messages within the reporter. I should be able to get what I need with MegaLinter & Azure PR comment status by the use of FAIL_IF_UPDATED_SOURCES 😎

@lukelloyd1985
Copy link
Contributor Author

@nvuillam, I've moved away from messing with MegaLinter output status and just put coloured error messages within the reporter. I should be able to get what I need with MegaLinter & Azure PR comment status by the use of FAIL_IF_UPDATED_SOURCES 😎

So FAIL_IF_UPDATED_SOURCES doesn't seem to do what I want but not sure if it's a bug or my understanding is incorrect?

It shows as failed in the console log & exits the pipeline:
image

However the Azure PR comment shows as success 🤷‍♀️
image

@karl18
Copy link

karl18 commented Oct 31, 2024

Hi @lukelloyd1985 quite interesting to see you contributing for AUTO_FIX \ APPLY_FIXES to make it working on Azure Devops. I believe it's a great missing feature which some people out there waiting to have it supported on MegaLinter. I also wanted to contribute the past days but was very busy with some tasks. Now, I've got a couple of questions here:

  1. Did it work on your side without doing specific conf. to the pipeline?
  2. Does it work when specifyin both VALIDATE_ALL_CODEBASE: Ture or false?
  3. Does it work with all linters?
  4. Lastly, How could I give a hand here?

Some cool guy was trying it out as well, but not sure if that worked with him or not. but worth mentioning https://etienne.deneuve.xyz/2023/07/28/autofix-megalinter-azure-devops/ @nvuillam

But nice to see that @lukelloyd1985 is embedding it already inside the code:

repo.config_writer().set_value("user", "name", "MegaLinter").release()
repo.config_writer().set_value("user", "email", "[email protected]").release()

@lukelloyd1985
Copy link
Contributor Author

Hi @lukelloyd1985 quite interesting to see you contributing for AUTO_FIX \ APPLY_FIXES to make it working on Azure Devops. I believe it's a great missing feature which some people out there waiting to have it supported on MegaLinter. I also wanted to contribute the past days but was very busy with some tasks. Now, I've got a couple of questions here:

Thanks for the enthusiasm, yes it's certainly something I am keen to get working to close the loop on an already great tool :)

  1. Did it work on your side without doing specific conf. to the pipeline?

Nothing special about the config needed, just follow https://megalinter.io/beta/install-azure/

Just waiting for this PR to be merged so I can test the next stage (there's been a few iterations fixing each error that's found to date)

  1. Does it work when specifyin both VALIDATE_ALL_CODEBASE: Ture or false?

I've only been working with it set to True for my needs but it shouldn't make a difference as using git add with update which will only commit modified tracked files

  1. Does it work with all linters?

Again as above it should work with any linter that has changed any file with git add with update being used

  1. Lastly, How could I give a hand here?

I think next stage is to get this PR merged so it can be tested to see what works and/or what errors are found. If/when it's working some support with testing different scenarios would be useful. Also do you have experience with other git platforms e.g. GitHub/Bitbucket etc as those would need testing too?

Some cool guy was trying it out as well, but not sure if that worked with him or not. but worth mentioning https://etienne.deneuve.xyz/2023/07/28/autofix-megalinter-azure-devops/ @nvuillam

But nice to see that @lukelloyd1985 is embedding it already inside the code:

repo.config_writer().set_value("user", "name", "MegaLinter").release()
repo.config_writer().set_value("user", "email", "[email protected]").release()

Yeah whatever can be put in code is going to be better I think, less to configure on pipelines and means all git platforms operate the same.

Still need to get FAIL_IF_UPDATED_SOURCES working with it too so that if git fails it causes an error and PR comment shows the error/failure

@lukelloyd1985
Copy link
Contributor Author

@nvuillam, I've moved away from messing with MegaLinter output status and just put coloured error messages within the reporter. I should be able to get what I need with MegaLinter & Azure PR comment status by the use of FAIL_IF_UPDATED_SOURCES 😎

So FAIL_IF_UPDATED_SOURCES doesn't seem to do what I want but not sure if it's a bug or my understanding is incorrect?

It shows as failed in the console log & exits the pipeline: image

However the Azure PR comment shows as success 🤷‍♀️ image

@nvuillam what are your thoughts on FAIL_IF_UPDATED_SOURCES and PR comment status?

Can this PR be merged into alpha so I can test the next stage of git auto fixes?

Copy link
Collaborator

@echoix echoix left a comment

Choose a reason for hiding this comment

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

It's for alpha, so I'm taking the liberty to unblock you, it seems fine enough for me anyways (even if it wasn't for alpha). Nicolas can continue thinking about the direction of it when it will be time to merge alpha back

@echoix
Copy link
Collaborator

echoix commented Oct 31, 2024

@lukelloyd1985 maybe adress cspell not recognizing some words in the updated sources reporter

@nvuillam nvuillam merged commit 1ed299a into oxsecurity:alpha Nov 1, 2024
3 of 5 checks passed
@lukelloyd1985
Copy link
Contributor Author

@lukelloyd1985 maybe adress cspell not recognizing some words in the updated sources reporter

@echoix unless I'm being thick where can I see the failed cspell tests?

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