Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

chore: cache results of vulnerability scans #621

Merged
merged 20 commits into from
Dec 18, 2018

Conversation

kuzhelov
Copy link
Contributor

@kuzhelov kuzhelov commented Dec 17, 2018

Provided changes introduce caching strategy for vulnerability tests - this caching will allow us to stay within monthly quota limit (200 runs per month, while expectation for introduced strategy is 30-60).

Caching Strategy

The strategy is the following:

  • for each yarn.lock
    • run vulnerability scan if there were no any made today, cache result
    • otherwise (if today's cached result present) just skip vulnerability tests

@DustyTheBot
Copy link
Collaborator

Warnings
⚠️ There are no updates provided to CHANGELOG. Ensure there are no publicly visible changes introduced by this PR.

Generated by 🚫 dangerJS

- run:
name: Vulnerability Tests
command: yarn test:vulns
- save_cache:
key: v1-vuln-scans-{{ checksum "yarn.lock" }}-{{ epoch }}
Copy link
Member

@layershifter layershifter Dec 18, 2018

Choose a reason for hiding this comment

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

Is it okay that we restoring a cache with a key without the epoch variable?

Copy link
Contributor Author

@kuzhelov kuzhelov Dec 18, 2018

Choose a reason for hiding this comment

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

this is necessary because otherwise Circle CI won't update the cache entry if it has existed before (this is its feature). To avoid this 'rewrite ban' the following strategy was suggested: https://discuss.circleci.com/t/add-mechanism-to-update-existing-cache-key/9014/12

Copy link
Member

Choose a reason for hiding this comment

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

As we discussed, please add a small comment with this link before the key line 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed to introduce comment for that


const ensureDirExists = path => {
if (!fs.existsSync(path)) {
sh(`mkdir -p ${path}`)
Copy link
Member

Choose a reason for hiding this comment

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

fs.mkdirSync(path, { recursive: true })?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for that we need Node LTS v10 (and this is, actually, the step we should make). Agreed to defer it to the follow-up PR, to be absolutely sure that all the necessary accompanying adjustments to the code will be made


const getScanResultsDirPath = () => {
return paths.base(SCAN_RESULTS_DIR_NAME)
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need it? As I see it can be a simple variable

const now = new Date()

const fileName = `snyk-scanned-${now.getUTCFullYear()}-${now.getUTCMonth() +
1}-${now.getUTCDate()}`
Copy link
Member

Choose a reason for hiding this comment

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

Can we split this to multiple variables? To avoid multiline there

@layershifter layershifter added the needs author feedback Author's opinion is asked label Dec 18, 2018
Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

I left small questions there. I prefer to use pure bash commands for such things, but it will be hard to debug them for others.

Let's ship it.

@kuzhelov kuzhelov added ready for merge and removed needs author feedback Author's opinion is asked labels Dec 18, 2018
@kuzhelov kuzhelov merged commit 359a083 into master Dec 18, 2018
@kuzhelov kuzhelov deleted the feat/vulnerability-scan-results-caching branch December 18, 2018 11:50
mnajdova pushed a commit that referenced this pull request Dec 19, 2018
* implement caching strategy

* adjust file name of scan marker

* add yarn lock hash to marker file name

* add change to build config

* fix dir name in build config

* improve caching strategy

* just restore cache

* temporary remove lint and tests

* try

* fix caching strategy

* try

* try

* try

* try epoch

* create file on scan

* return lint and test steps

* introduce comment for the caching approach taken

* remove unnecessary function

* simplify expression for marker file name
mnajdova pushed a commit that referenced this pull request Dec 19, 2018
* feat(menuItem: add menu prop)

* Only one submenu open at a time

* Fix bug: submenu noe closes on clicking a menuItem with no submenu

* Code cleanup

* Remove the activeIndex prop passed to the menuItem

* Remove Popup and implement submenu without it

* initial keyboard support

* left/right arrow handling

* simplify keyboard handlers

* -fixed import in the MenuItem

* -added submenuIndicator
-small fixes in the styles and the way the submenu is generated

* -clicking on leaf element should close the submenus (the same should be done for enter/space)
-applied consistent (left-right) navigation for horizontal menu and (up-down) navigation for vertical menu

* -implemented outside click to close all menus
-implemented enter key on leaf menu item to close the menu

* -fixed import

* -refactored MenuItem handlers - fixed issues
-removed onClick handler for the Menu (not necessary for now)
-added onKeyDown in the creation of the MenuItem in the Menu component for handling the action prop

* -added setActiveIndex callback and removed onKeyDown in the creation of MenuItems in the Menu component

* -right arrow key is closing the submenus and goes to the next element if the menu is horizontal, or is focusing the first MenuItem if it is vertical

* -handled left arrow key

* -changed ref
-focus trap wip

* -added Ref component instead of using the itemRef on the ElementType
-removed subscription for focus

* -fixes

* -moved ref //TODO: figure out tests failing

* -close menu on outside focus

* -improved comments

* -fixed escape key not focusing the active element
-changed parentRef to inSubmenu boolean

* -fixing key problems
-added dependency for generating id

* -refactored submenuRef element

* fix broken tests

* -added comments in the tests
-changed the submenuDomElement so submenuRef

* -renamed inSubmenu to submenu prop in the Menu

* -fixed with the auto-controlled prop in the Menu

* -added state interface in the Menu
-improved menu variables' names

* -fixed variables in examples

* -remove state initialization in the MenuItem component

* -added new handler for escape
-changed submenu examples titles

* -refactored conditions using doesNodeContainClick

* -renamed submenu* props to menu in the MenuItem component
-changed setActiveIndex with onActiveChanged
-introduced different styles for the hovering vs active elements

* -improved example
-fixed issue with the condition for the active prop

* -exported MenuState
-added correct typings to the menuStyles

* -fixed underlined active + hovered style

* -fixed border corner clipped by adding custom styles for the first child menu items and the last child menu items in vertical menu

* -addressed comments on PR

* chore: prepare release 0.15.0 [ci skip]

* 0.15.0

* fix(Prototype): Fix Popover prototype after breaking changes (#623)

* chore: cache results of vulnerability scans (#621)

* implement caching strategy

* adjust file name of scan marker

* add yarn lock hash to marker file name

* add change to build config

* fix dir name in build config

* improve caching strategy

* just restore cache

* temporary remove lint and tests

* try

* fix caching strategy

* try

* try

* try

* try epoch

* create file on scan

* return lint and test steps

* introduce comment for the caching approach taken

* remove unnecessary function

* simplify expression for marker file name

* feat(text): color prop (#597)

* feat(text): color prop

* addressed comments

* changelog

* amended changelog

* made text color override other props that change color

* feat(header): header and header description color prop (#628)

* feat(header): header and header description color prop

* changelog

* fixed examples

* addressed PR comments

* fix(Popup): allow to 'detach' from trigger and RTL adjustments (#612)

* introduce offset prop

* correct description of supported values

* update changelog

* introduce fix

* ensure RTL is properly applied to complex offset expressions

* rename method to make logic more expressive

* add unit tests

* remove unnecessary grid props from offset example

* update changelog

* fix(SelectableList): Items in list should be selectable (#566)

* Reflect which item is selected in list

* Make list derived from autocontrolled component

* small fix

* Update ListExampleSelection.tsx

* Update ListExampleSelection.shorthand.tsx

* Small improvement

* Rename *ItemIndex -> *Index

* Names refactoring

* Minor improvements

* update changelog

* Add onSelectedIndexChange

* Add some tests

* Small improvements afer CR

* Small improvements afer CR

* Small improvements afer CR

* create focus handler when List is constructed

* fix changelog

* changelog

* docs(Examples): allow to use TS in examples (#617)

* docs(Examples): allow to use TS in examples

* add jsdoc

* fix typo

* rename file

* add comment

* `createExample` to `createExampleSourceCode`

* rework with `path.relative()`

* remove JSON files on remove tsx

* create getRelativePathToSource function

* -updated changelog

* -updated changelog

* -updated changelog with breaking changes
-removed TODO comment

* -fixed imports in examples
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants