Skip to content

Fix miscellaneous ci issues #78

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.

Sign up for GitHub

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

Merged
aminya merged 9 commits into from
Aug 14, 2020
Merged

Fix miscellaneous ci issues #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e0abca6
CI: Fix Windows job names
DeeDeeG Aug 4, 2020
9c138f8
CI: Delete outdated comment in platforms/linux.yml
DeeDeeG Aug 4, 2020
72e9dc6
CI: Don't pass REPO variables via `env:` yml
DeeDeeG Aug 4, 2020
d77c2a2
CI: Set appropriate default values for REPO vars
DeeDeeG Aug 4, 2020
d167180
:shirt: CI: Fix JS lints (prettier)
DeeDeeG Aug 4, 2020
a24e93a
CI: Update displayNames for Windows jobs
DeeDeeG Aug 5, 2020
d5c8456
CI: Use more-compact "default var" syntax in JS
DeeDeeG Aug 5, 2020
1ee74e5
:shirt: Fix lints again
DeeDeeG Aug 5, 2020
cfd4a0b
CI: Drop old workaround, refactor script blocks
DeeDeeG Aug 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions script/vsts/get-release-version.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ const request = require('request-promise-native');
const repositoryRootPath = path.resolve(__dirname, '..', '..');
const appMetadata = require(path.join(repositoryRootPath, 'package.json'));

const REPO_OWNER = process.env.REPO_OWNER;
const NIGHTLY_RELEASE_REPO = process.env.NIGHTLY_RELEASE_REPO;
const REPO_OWNER = process.env.REPO_OWNER || 'atom';
const NIGHTLY_RELEASE_REPO =
process.env.NIGHTLY_RELEASE_REPO || 'atom-nightly-releases';

const yargs = require('yargs');
Copy link
Copy Markdown
Member

@aminya aminya Aug 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a reason that MAIN_REPO is missing here? Just want to check if we haven't missed it.

Copy link
Copy Markdown
Member Author

@DeeDeeG DeeDeeG Aug 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The script authors didn't use "the name of the main repo" anywhere in the script. I suppose they had no need for it.

(Where they use the string 'atom', they mean "the repo owner"; the org/user whose account the [nightly] repo is under.)

And the linter gets upset if you define a variable and don't use it.

Copy link
Copy Markdown
Member Author

@DeeDeeG DeeDeeG Aug 5, 2020

Choose a reason for hiding this comment

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

https://github.com/atom-ide-community/atom/blob/23ad7fe8575e8419e08ede6e2ebe110e8708e725/script/vsts/get-release-version.js#L32

This is the only place in the file any of these vars is needed.

Copy link
Copy Markdown
Member Author

@DeeDeeG DeeDeeG Aug 5, 2020

Choose a reason for hiding this comment

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

By the way, from reading that line, it appears if our Nightly repo goes stale, and lacks recent tags, our auto drafts of releases (if we use that feature) will not advance their version number. So we would perhaps have to manually fix the suggested version number?

Something to keep in mind. Some of Atom's release CI steps are very interconnected with the Nightly repo.

const argv = yargs
Expand Down
7 changes: 4 additions & 3 deletions script/vsts/lib/release-notes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ const octokit = require('@octokit/rest')();
const changelog = require('pr-changelog');
const childProcess = require('child_process');

const REPO_OWNER = process.env.REPO_OWNER;
const MAIN_REPO = process.env.MAIN_REPO;
const NIGHTLY_RELEASE_REPO = process.env.NIGHTLY_RELEASE_REPO;
const REPO_OWNER = process.env.REPO_OWNER || 'atom';
const MAIN_REPO = process.env.MAIN_REPO || 'atom';
const NIGHTLY_RELEASE_REPO =
process.env.NIGHTLY_RELEASE_REPO || 'atom-nightly-releases';

module.exports.getRelease = async function(releaseVersion, githubToken) {
if (githubToken) {
Expand Down
15 changes: 2 additions & 13 deletions script/vsts/nightly-release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,11 @@ jobs:
pool:
vmImage: 'ubuntu-latest'
steps:
# This has to be done separately because VSTS inexplicably
# exits the script block after `npm ci` completes.
- script: |
cd script/vsts
npm ci
displayName: npm ci
- script: node script/vsts/get-release-version.js --nightly
node get-release-version.js --nightly
name: Version
env:
REPO_OWNER: $(REPO_OWNER)
NIGHTLY_RELEASE_REPO: $(NIGHTLY_RELEASE_REPO)

# Import OS-specific build definitions
- template: platforms/windows.yml
Expand All @@ -27,7 +21,7 @@ jobs:

dependsOn:
- GetReleaseVersion
- Windows
- Windows_RendererTests
- Linux
- macOS_tests

Expand All @@ -37,8 +31,6 @@ jobs:
steps:
- template: platforms/templates/preparation.yml

#This has to be done separately because VSTS inexplicably
#exits the script block after `npm ci` completes.
- script: |
cd script/vsts
npm ci
Expand All @@ -59,9 +51,6 @@ jobs:
ATOM_RELEASES_S3_SECRET: $(ATOM_RELEASES_S3_SECRET)
ATOM_RELEASES_S3_BUCKET: $(ATOM_RELEASES_S3_BUCKET)
PACKAGE_CLOUD_API_KEY: $(PACKAGE_CLOUD_API_KEY)
REPO_OWNER: $(REPO_OWNER)
MAIN_REPO: $(MAIN_REPO)
NIGHTLY_RELEASE_REPO: $(NIGHTLY_RELEASE_REPO)
displayName: Create Nightly Release
- job: bump_dependencies
displayName: Bump Dependencies
Expand Down
1 change: 0 additions & 1 deletion script/vsts/platforms/linux.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ jobs:
variables:
ReleaseVersion: $[ dependencies.GetReleaseVersion.outputs['Version.ReleaseVersion'] ]
pool:
# This image is used to host the Docker container that runs the build
vmImage: ubuntu-16.04

steps:
Expand Down
6 changes: 4 additions & 2 deletions script/vsts/platforms/windows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
jobs:
- job: Windows
- job: Windows_build
displayName: Windows build
Comment on lines +2 to +3
Copy link
Copy Markdown
Member

@aminya aminya Aug 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Please revert this. Make it Windows to keep it simple. It is still called Windows on upstream.

Copy link
Copy Markdown
Member Author

@DeeDeeG DeeDeeG Aug 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I was copying the macOS build job label that upstream uses for macOS, where the CI is split up between a "build" job and a "parallel test" job. Just like we introduced for Windows at this fork.

So maybe upstream will eventually take this? I do think they want parallel testing on Windows x64. (I think they might be more likely to take it if it is separated out from the "move lots of stuff to templates" changes. But I'm not them so I can't read their minds about that. It would be most welcome if they would comment on our PR.)

Copy link
Copy Markdown
Member

@aminya aminya Aug 5, 2020

Choose a reason for hiding this comment

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

This job is called on other files too like Release job. After all, it is just a name, I don't think it is important. 😄 If you want to change it you change it everywhere.

dependsOn: GetReleaseVersion
timeoutInMinutes: 180
strategy:
Expand Down Expand Up @@ -71,7 +72,8 @@ jobs:
condition: and(succeeded(), eq(variables['IsReleaseBranch'], 'true'))

- job: Windows_RendererTests
dependsOn: Windows
displayName: Windows
dependsOn: Windows_build
Comment on lines +75 to +76
Copy link
Copy Markdown
Member

@aminya aminya Aug 5, 2020

Choose a reason for hiding this comment

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

Please revert this.

timeoutInMinutes: 180
strategy:
maxParallel: 2
Expand Down
8 changes: 1 addition & 7 deletions script/vsts/pull-requests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,11 @@ jobs:
pool:
vmImage: 'ubuntu-latest'
steps:
# This has to be done separately because VSTS inexplicably
# exits the script block after `npm ci` completes.
- script: |
cd script/vsts
npm ci
displayName: npm ci
- script: node script/vsts/get-release-version.js
node get-release-version.js
name: Version
env:
REPO_OWNER: $(REPO_OWNER)
NIGHTLY_RELEASE_REPO: $(NIGHTLY_RELEASE_REPO)

# Import OS-specific build definitions
- template: platforms/windows.yml
Expand Down
18 changes: 2 additions & 16 deletions script/vsts/release-branch-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,11 @@ jobs:
pool:
vmImage: 'ubuntu-latest'
steps:
# This has to be done separately because VSTS inexplicably
# exits the script block after `npm ci` completes.
- script: |
cd script/vsts
npm ci
displayName: npm ci
- script: node script/vsts/get-release-version.js
node get-release-version.js
name: Version
env:
REPO_OWNER: $(REPO_OWNER)
NIGHTLY_RELEASE_REPO: $(NIGHTLY_RELEASE_REPO)

# Import OS-specific build definitions.
- template: platforms/windows.yml
Expand All @@ -32,7 +26,7 @@ jobs:

dependsOn:
- GetReleaseVersion
- Windows
- Windows_RendererTests
- Linux
- macOS_tests

Expand All @@ -42,8 +36,6 @@ jobs:
IsSignedZipBranch: $[ dependencies.GetReleaseVersion.outputs['Version.IsSignedZipBranch'] ]

steps:
# This has to be done separately because VSTS inexplicably
# exits the script block after `npm ci` completes.
- script: |
cd script/vsts
npm ci
Expand All @@ -66,9 +58,6 @@ jobs:
ATOM_RELEASES_S3_SECRET: $(ATOM_RELEASES_S3_SECRET)
ATOM_RELEASES_S3_BUCKET: $(ATOM_RELEASES_S3_BUCKET)
PACKAGE_CLOUD_API_KEY: $(PACKAGE_CLOUD_API_KEY)
REPO_OWNER: $(REPO_OWNER)
MAIN_REPO: $(MAIN_REPO)
NIGHTLY_RELEASE_REPO: $(NIGHTLY_RELEASE_REPO)
displayName: Create Draft Release
condition: and(succeeded(), eq(variables['Atom.AutoDraftRelease'], 'true'), eq(variables['IsReleaseBranch'], 'true'))

Expand All @@ -79,8 +68,5 @@ jobs:
ATOM_RELEASES_S3_KEY: $(ATOM_RELEASES_S3_KEY)
ATOM_RELEASES_S3_SECRET: $(ATOM_RELEASES_S3_SECRET)
ATOM_RELEASES_S3_BUCKET: $(ATOM_RELEASES_S3_BUCKET)
REPO_OWNER: $(REPO_OWNER)
MAIN_REPO: $(MAIN_REPO)
NIGHTLY_RELEASE_REPO: $(NIGHTLY_RELEASE_REPO)
displayName: Upload CI Artifacts to S3
condition: and(succeeded(), eq(variables['IsSignedZipBranch'], 'true'))
7 changes: 4 additions & 3 deletions script/vsts/upload-artifacts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ const uploadLinuxPackages = require('./lib/upload-linux-packages');

const CONFIG = require('../config');

const REPO_OWNER = process.env.REPO_OWNER;
const MAIN_REPO = process.env.MAIN_REPO;
const NIGHTLY_RELEASE_REPO = process.env.NIGHTLY_RELEASE_REPO;
const REPO_OWNER = process.env.REPO_OWNER || 'atom';
const MAIN_REPO = process.env.MAIN_REPO || 'atom';
const NIGHTLY_RELEASE_REPO =
process.env.NIGHTLY_RELEASE_REPO || 'atom-nightly-releases';

const yargs = require('yargs');
const argv = yargs
Expand Down