-
Notifications
You must be signed in to change notification settings - Fork 53
CHANGE: @W-18097146@ Updates for GA #1771
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
Conversation
| ref: 'dev', | ||
| inputs: { | ||
| "release-type": "patch" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey this is great news! We'll do this instead of manually creating it then for this month. Great find.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for this month we want to run it as patch. Good call.
| name: create-release-branch | ||
| on: | ||
| workflow_dispatch: | ||
| inputs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I also thought it prudent to add in the manual option back in case we need to re-run (for whatever reason)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we definitely need this now.
| - name: Locally increment version | ||
| run: | | ||
| npm --no-git-tag-version version prerelease --preid beta | ||
| # A workflow dispatch event lets the user specify what release type they want. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this on dev-4 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this makes it so the automated monthly execution defaults to minor. It won't cause a problem here because patch is being explicitly passed in, and it's technically also unnecessary in v4 because minor gets explicitly passed in there too.
But it's still a nice thing to have.
.github/workflows/run-tests.yml
Outdated
| run: | | ||
| # We need to determine the Tarball's name first. | ||
| TARBALL_NAME=$(ls ~/downloads/tarball | grep salesforce-plugin-code-analyzer-5\\.0\\.0-beta\\.[0-9]*\\.tgz) | ||
| TARBALL_NAME=$(ls ~/downloads/tarball | grep salesforce-plugin-code-analyzer-*.*.tgz) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regex is not my favorite, but I think this will work. Let me know if there's a better approach here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- means 0 or more... and it refers to the character it precedes.
So when you do-*that means zero or more-characters. This isn't what you want.
. means any character. So .* means zero or more of any character.
\. means the literal . but we need to have \. so we actually provide to grep \. because \ is the escape character and thus \ is interpreted as ''.
So what you really want here is:
TARBALL_NAME=$(ls ~/downloads/tarball | grep salesforce-plugin-code-analyzer-.*\\.tgz)
alternatively if we want to be more strict and require 5.x.y we would have:
TARBALL_NAME=$(ls ~/downloads/tarball | grep salesforce-plugin-code-analyzer-5\\.[0-9]*\\.[0-9]*\\.tgz)
does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that does, thank you! I like the looser version here since we're using it in a build option, and it would be nice not to have to update this when the major version changes again (since this was not an intuitive find for me either)
.git2gus/config.json
Outdated
| { | ||
| "productTag": "a1aB0000000c7jnIAA", | ||
| "defaultBuild": "[SFCA] Scanner 4.0", | ||
| "defaultBuild": "[SFCA] Code Analyzer 5.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put a message in slack about this. We need whatever build we put here to first live in GUS.
| ref: 'dev', | ||
| inputs: { | ||
| "release-type": "patch" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey this is great news! We'll do this instead of manually creating it then for this month. Great find.
| await github.rest.actions.createWorkflowDispatch({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| workflow_id: 'daily-smoke-tests.yml', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused. Isn't this an infinite loop? Isn't line 11 already running the smoke test? This new section will call this same file again wouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, I think this needs a re-write. Let me take another stab at that 😄
.github/workflows/run-tests.yml
Outdated
| run: | | ||
| # We need to determine the Tarball's name first. | ||
| TARBALL_NAME=$(ls ~/downloads/tarball | grep salesforce-plugin-code-analyzer-5\\.0\\.0-beta\\.[0-9]*\\.tgz) | ||
| TARBALL_NAME=$(ls ~/downloads/tarball | grep salesforce-plugin-code-analyzer-*.*.tgz) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- means 0 or more... and it refers to the character it precedes.
So when you do-*that means zero or more-characters. This isn't what you want.
. means any character. So .* means zero or more of any character.
\. means the literal . but we need to have \. so we actually provide to grep \. because \ is the escape character and thus \ is interpreted as ''.
So what you really want here is:
TARBALL_NAME=$(ls ~/downloads/tarball | grep salesforce-plugin-code-analyzer-.*\\.tgz)
alternatively if we want to be more strict and require 5.x.y we would have:
TARBALL_NAME=$(ls ~/downloads/tarball | grep salesforce-plugin-code-analyzer-5\\.[0-9]*\\.[0-9]*\\.tgz)
does that make sense?
| @@ -0,0 +1,29 @@ | |||
| #TODO: remove v4 with April Release | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephen-carter-at-sf I opted to keep this separate for easy deletion. I'm not quite sure why we need the script to invoke the tests, but I updated daily-smoke-tests to just work off of dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly were you confused about? I'm not quite sure I understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cron based jobs can only run from dev. So from dev we dispatch the actual v4 script from the dev-4 branch. What you have here is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Do we want to have separate issues for the run, rules, and config commands like we do for the v4 commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should. Besides that is a separate discussion outside the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering the same thing @jfeingold35 - we could add that as we get additional feedback indicating we need more info per command, but I do like the option to make it simpler too (that's my plan for the follow-up WI 18163361)
| ref: 'dev', | ||
| inputs: { | ||
| "release-type": "patch" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for this month we want to run it as patch. Good call.
| name: create-release-branch | ||
| on: | ||
| workflow_dispatch: | ||
| inputs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we definitely need this now.
| - name: Locally increment version | ||
| run: | | ||
| npm --no-git-tag-version version prerelease --preid beta | ||
| # A workflow dispatch event lets the user specify what release type they want. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this makes it so the automated monthly execution defaults to minor. It won't cause a problem here because patch is being explicitly passed in, and it's technically also unnecessary in v4 because minor gets explicitly passed in there too.
But it's still a nice thing to have.
| jobs: | ||
| # We want to prevent cross-contamination between the 4.x and 5.x pipelines. So we should prevent PRs | ||
| # based on this flow to merge into `dev-4` or `main-4`. | ||
| # TODO: Remove this after the April release, since we won't care after that to maintain v4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: You'll also need to remove this job as a Required job for the branch protection rule.
.github/workflows/run-tests.yml
Outdated
| path: ~/downloads/tarball | ||
| - name: Echo tarball download location | ||
| run: echo ${{ steps.download.outputs.download-path }} | ||
| # TODO: update ASAP after April Release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the update that needs to happen here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh good call - this isn't necessary with the latest change. I'll remove!
| run: | | ||
| # We need to determine the Tarball's name first. | ||
| TARBALL_NAME=$(ls ~/downloads/tarball | grep salesforce-plugin-code-analyzer-5\\.0\\.0-beta\\.[0-9]*\\.tgz) | ||
| TARBALL_NAME=$(ls ~/downloads/tarball | grep salesforce-plugin-code-analyzer-.*\\.tgz) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend greping for salesforce-plugin-code-analyzer-5\\.[0-9]*\\.[0-9]*\\.tgz.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one I realized we'll need to wait until after we go GA, but we can clean that up then if you'd like 👍
| workflow_id: 'daily-smoke-tests.yml', | ||
| ref: 'dev-4' | ||
| }); | ||
| branch: dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request: Could you add a comment covering the explanation I gave the other day? i.e.,
We run the daily smoke tests against 'dev' to validate that the code currently in development is still valid
or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point to clarify!
| @@ -0,0 +1,29 @@ | |||
| #TODO: remove v4 with April Release | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly were you confused about? I'm not quite sure I understand.

Remove the word "Beta" and any language around going GA. Update Github actions to be as ready for v5 as we can while we still need to support v4.